Cell should be the transpose when communicating between siesta and i-PI

Bug #1835196 reported by Mariana Rossi
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Siesta
Fix Committed
Medium
Nick Papior
4.0
Fix Committed
Medium
Nick Papior
4.1
Fix Released
Medium
Nick Papior

Bug Description

Because of the matrix definitions in i-PI and SIESTA, currently the cell that is being passed to SIESTA is the transpose of what it should be. This only affects non-orthogonal cells. The following fix in the routine iosockets.F90 seems to fix the problem in my tests:

From line 197:

  cell = RESHAPE( c, (/3,3/) )
+ cell = TRANSPOSE(cell)

That's all.

Cheers,

Mariana

Related branches

Revision history for this message
Nick Papior (nickpapior) wrote :

Thanks Mariana!

This will be fixed in 4.0 and updated in all versions! Thanks!

Changed in siesta:
status: New → In Progress
assignee: nobody → Nick Papior (nickpapior)
importance: Undecided → Medium
Revision history for this message
Mariana Rossi (mahrossi) wrote : Re: [Bug 1835196] Re: Cell should be the transpose when communicating between siesta and i-PI

Hi Nick,

Further, I just literally noticed yesterday that in case siesta would pass the stress tensor to do a NPT simulation, this also has to be fixed.
I did not yet have time to test is, but essentially the line ~275-276 in iosockets.F90:

! Find virial tensor for i-pi, in master's units
  vir(:) = -s * cellv * fdf_convfac( siesta_xunit, master_xunit )**3

should also be exchanged by

! Find virial tensor for i-pi, in master's units
  vir(:) = -TRANSPOSE(s) * cellv * fdf_convfac( siesta_xunit, master_xunit )**3

I think it is worth implementing this fix even without further testing as it is the only way everything is consistent.

Cheers and many thanks,

Mariana

> On 29. Jul 2019, at 11:16, Nick Papior <email address hidden> wrote:
>
> ** Changed in: siesta/4.0
> Status: In Progress => Fix Committed
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1835196
>
> Title:
> Cell should be the transpose when communicating between siesta and
> i-PI
>
> Status in Siesta:
> In Progress
> Status in Siesta 4.0 series:
> Fix Committed
> Status in Siesta 4.1 series:
> In Progress
>
> Bug description:
> Because of the matrix definitions in i-PI and SIESTA, currently the
> cell that is being passed to SIESTA is the transpose of what it should
> be. This only affects non-orthogonal cells. The following fix in the
> routine iosockets.F90 seems to fix the problem in my tests:
>
> From line 197:
>
> cell = RESHAPE( c, (/3,3/) )
> + cell = TRANSPOSE(cell)
>
> That's all.
>
> Cheers,
>
> Mariana
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/siesta/+bug/1835196/+subscriptions

---------------------------------------------------------------
Dr. Mariana Rossi e-mail: <email address hidden> <mailto:<email address hidden>>.de
Faradayweg 4-6 phone: +49 30 84134839
Theory Department
14195 Berlin
Germany

http://th.fhi-berlin.mpg.de/groups/sabia/ <http://th.fhi-berlin.mpg.de/groups/sabia/>
 <http://www.fhi-berlin.mpg.de/~rossi/>
-------- key:1-0.0735-11600-23.05:fhi --------------

Revision history for this message
Nick Papior (nickpapior) wrote :

Actually, I am not so sure that i-pi uses the virial tensor as the transpose in its implementation.

The reason for cell to be transposed is because i-pi uses the cell vectors transposed in the python implementation (in my opinion this is an odd choice, but never mind). However, the virial matrix is never used as such. In fact it is automatically truncated to the Voigt representation once received from the socket.

Two things that may be valuable for future reference:

1) the virial tensor is symmetric, so it shouldn't matter either way
2) the i-pi/drivers/driver.f90 uses the transpose of the virial matrix. However, in that implementation the virial matrix is only calculated in the (1,[1,2,3]), (2,[2,3]), (3, [3]) values (which are not the standard fortran indexing). This is my main motivation for *not* doing this with the virial matrix since the i-pi implementation does this (after retrieval: see engine/forces.py: get_vir) vir[1, 0] = 0, vir[2, 0:2] = 0.

Thanks for getting me to look up this.

Revision history for this message
Mariana Rossi (mahrossi) wrote :

Hi Nick,

Yes — I agree that if Siesta always calculates the full tensor and not only the upper or lower triangular piece of it, there should be no problem (and there should be equally no problem with transposing it, right?)
I agree with your assessment, and I had indeed not tested this part so all can totally be fine the way it is (and my knowledge of Siesta is rather shallow at the moment ;-) ). It just came to my mind that there could
be trouble also there.

All the best,

Mariana

> On 29. Jul 2019, at 13:57, Nick Papior <email address hidden> wrote:
>
> Actually, I am not so sure that i-pi uses the virial tensor as the
> transpose in its implementation.
>
> The reason for cell to be transposed is because i-pi uses the cell
> vectors transposed in the python implementation (in my opinion this is
> an odd choice, but never mind). However, the virial matrix is never used
> as such. In fact it is automatically truncated to the Voigt
> representation once received from the socket.
>
> Two things that may be valuable for future reference:
>
> 1) the virial tensor is symmetric, so it shouldn't matter either way
> 2) the i-pi/drivers/driver.f90 uses the transpose of the virial matrix. However, in that implementation the virial matrix is only calculated in the (1,[1,2,3]), (2,[2,3]), (3, [3]) values (which are not the standard fortran indexing). This is my main motivation for *not* doing this with the virial matrix since the i-pi implementation does this (after retrieval: see engine/forces.py: get_vir) vir[1, 0] = 0, vir[2, 0:2] = 0.
>
>
> Thanks for getting me to look up this.
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1835196
>
> Title:
> Cell should be the transpose when communicating between siesta and
> i-PI
>
> Status in Siesta:
> In Progress
> Status in Siesta 4.0 series:
> Fix Committed
> Status in Siesta 4.1 series:
> In Progress
>
> Bug description:
> Because of the matrix definitions in i-PI and SIESTA, currently the
> cell that is being passed to SIESTA is the transpose of what it should
> be. This only affects non-orthogonal cells. The following fix in the
> routine iosockets.F90 seems to fix the problem in my tests:
>
> From line 197:
>
> cell = RESHAPE( c, (/3,3/) )
> + cell = TRANSPOSE(cell)
>
> That's all.
>
> Cheers,
>
> Mariana
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/siesta/+bug/1835196/+subscriptions

---------------------------------------------------------------
Dr. Mariana Rossi e-mail: <email address hidden> <mailto:<email address hidden>>.de
Faradayweg 4-6 phone: +49 30 84134839
Theory Department
14195 Berlin
Germany

http://th.fhi-berlin.mpg.de/groups/sabia/ <http://th.fhi-berlin.mpg.de/groups/sabia/>
 <http://www.fhi-berlin.mpg.de/~rossi/>
-------- key:1-0.0735-11600-23.05:fhi --------------

Revision history for this message
Nick Papior (nickpapior) wrote :

No, agreed, there shouldn't be a problem of transposing it either. :)

Changed in siesta:
status: In Progress → Fix Committed
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.