Lammps: Use change_box when switching from orthogonal box to triclini… #25

jan-janssen · 2021-01-18T16:11:33Z

…c box

samwaseda · 2021-01-18T16:16:07Z

pyiron_atomistics/lammps/interactive.py

@@ -114,12 +114,18 @@ def interactive_cells_setter(self, cell):
            warnings.warn('set_relative() is deprecated as of 2020-02-26. It is not guaranteed from pyiron_atomistics vers. 0.3')


It's totally unrelated, but since this feature is badly deprecated, I guess we shouldn't forget to erase it after this PR.

samwaseda · 2021-01-18T16:17:36Z

pyiron_atomistics/lammps/interactive.py

@@ -114,12 +114,18 @@ def interactive_cells_setter(self, cell):
            warnings.warn('set_relative() is deprecated as of 2020-02-26. It is not guaranteed from pyiron_atomistics vers. 0.3')

        if is_skewed and is_scaled:
+            self._interactive_lib_command(
+                "change_box all triclinic"
+            )
            self._interactive_lib_command(
                "change_box all x final 0 %f y final 0 %f z final 0 %f \
                 xy final %f xz final %f yz final %f triclinic remap units box"


If we are setting change_box all triclinic in the command above, we can probably erase triclinic here (same for the command below).

coveralls · 2021-01-18T16:20:28Z

Pull Request Test Coverage Report for Build 495814342

2 of 6 (33.33%) changed or added relevant lines in 1 file are covered.
2 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-0.01%) to 64.615%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pyiron_atomistics/lammps/interactive.py	2	6	33.33%

Files with Coverage Reduction	New Missed Lines	%
pyiron_atomistics/lammps/interactive.py	2	35.42%

Totals
Change from base Build 495736873:	-0.01%
Covered Lines:	10538
Relevant Lines:	16309

💛 - Coveralls

samwaseda

This doesn't work if we go from a skewed box to an orthogonal box, because the shear components will be ignored (although to be more precise, this problem was already present before this PR).

jan-janssen · 2021-01-18T16:39:23Z

I need this fix for the old elastic constants job which is currently not working.

With the current master branch I get the following error message:

ERROR: Cannot change box tilt factors for orthogonal box (../change_box.cpp:249)

liamhuber

My Lammps-fu isn't good enough to just look at this and understand what the impact will be. I don't have any objections to the change, but it would be very helpful if the tests were also updated to demonstrate the behaviour you're trying to get.

samwaseda · 2021-01-18T17:54:30Z

I need this fix for the old elastic constants job which is currently not working.

With the current master branch I get the following error message:
ERROR: Cannot change box tilt factors for orthogonal box (../change_box.cpp:249)

Sorry I don't understand what you want to say with this.

jan-janssen · 2021-01-18T18:09:39Z

This doesn't work if we go from a skewed box to an orthogonal box, because the shear components will be ignored (although to be more precise, this problem was already present before this PR).

I agree that this is an issue which should be addressed but that is not part of this pull request.

samwaseda · 2021-01-18T20:16:13Z

I agree that this is an issue which should be addressed but that is not part of this pull request.

There I would slightly (but not totally) disagree. In the previous implementation it was not possible to go from orthogonal to skewed or other way around. If you make it possible to go from orthogonal to skewed with the new implementation I would find it more logical to make it also possible to go from skewed to orthogonal. Especially regarding the fact that going from skewed to orthogonal doesn’t even raise an error, so pyiron silently gives back wrong output and the user might never know that.

jan-janssen · 2021-01-18T21:18:17Z

In the previous implementation it was not possible to go from orthogonal to skewed or other way around.

In the previous implementation all structures were triclinic, this changed in pyiron/pyiron#1083 so all I want is to restore functionality we had before.

samwaseda · 2021-01-19T06:14:17Z

In the previous implementation all structures were triclinic, this changed in pyiron/pyiron#1083 so all I want is to restore functionality we had before.

That's perfectly fine by me, but the new implementation still doesn't rectify the problem that it's not possible to go from a skewed box to an orthogonal box.

jan-janssen · 2021-01-19T07:41:12Z

pyiron_atomistics/lammps/interactive.py

            self._interactive_lib_command(
                "change_box all x final 0 %f y final 0 %f z final 0 %f remap units box"
                % (lx, ly, lz)
            )
        else:  # is neither skewed nor scaled
+            self._interactive_lib_command(
+                "change_box all ortho"


@samwaseda Is this what you meant?

No this won't work either, because xy will still retain the previous value.

@samwaseda Then I am not sure what you want and prefer to merge this pull request and address your request in a separate pull request.

Wait "I don't understand what you're saying so I'm gonna merge it" is anyway not quite acceptable. Let me explain the problem one more time and then we're gonna talk about whether to still ignore the problem and merge it or not, ok?

So what pyiron does is set lx ly lz xy xz yz if the box is skewed, and lx ly z if the box is orthogonal. Imagine we have an orthogonal box at the beginning and go through the following case:

Set xy=0.1 -> run

Set xy=0 -> run

What happens inside pyiron is:

xy=0.1: box is skewed -> change_box lx ly lz xy xz yz -> run

xy=0: box is orthogonal -> change_box lx ly lz -> run

And as you can see in the second step, xy remains untouched, while pyiron should have actively set xy=0.

I hope this explanation was clear.

I understand the physical implications - I just say it was not working before and I do not know how to fix it, because we would have to read the current box inside lammps and then make a decision how to change this box which most likely leads to performance penalties. Therefore I suggest to merge the current pull request because it at least fixes the old elastic constant protocol developed by yury, which is not working with the current master branch.

I have the feeling that it's better to leave it then, because, again, going from skewed to orthogonal won't raise an error, but silently returns wrong output. So by making it possible to go from orthogonal to skewed with this PR, we might be making it even more difficult to realise where the problem comes from later.

So I would say, either we solve both problems or nothing. But I'll accept also other decisions if other people prefer a different way. What do you guys think? @liamhuber @pmrv @sudarsan-surendralal

Aha. Also from your other comment I understand a bit better. Failing silently is the worst of devils and should be exorcised immediately. I agree entirely that making it work in one direction but not the other makes this an even worse trap.

We definitely don't need the functionality to work both ways, but we should make it fail ASAP, especially if we now make it work one way.

Ok then maybe the quickest solution is to set change_box orthogonal, because then LAMMPS complains, right?

because then LAMMPS complains, right?

cough need tests cough 😉

liamhuber

I am still not tracking the Lammps super specifically, but I am trying to follow the discussion. Below is my current understanding. Ideal would be some test to ensure the desired pyiron behaviour, but I'm not going to force that issue.

Old

EDIT: Sam corrected my understanding
EDIT again: @samwaseda is the post correct now?

Change skew to orthogonal) ~~Fail with error~~ Silently does the wrong thing -- AAAHHH!
Change orthogonal to skew) ~~Fail with error~~ ~~Silently does the wrong thing -- AAAHHH!~~ Fails with error

New (unacceptable)

Change skew to orthogonal) Silently does the wrong thing
Change orthogonal to skew) Works

Requested

Change skew to orthogonal) Fails with error
Change orthogonal to skew) Works

samwaseda · 2021-01-19T09:00:34Z

Old

Change skew to orthogonal) Fail with error

No the old version also does the wrong thing silently.

jan-janssen · 2021-01-19T09:30:38Z

This starts to get fundamental. This pull request addresses one part of the issue above:

Old

Change orthogonal to skew - fails

New

Change orthogonal to skew - works

While the other changes would be nice to have they are not part of this pull request, mainly because I do not know how to get the information about the current structure from LAMMPS in an efficient way.

The tests for this part would require running lammps and I thought we agreed that we do not want to run simulation codes in the regular unit tests.

samwaseda · 2021-01-19T09:34:15Z

Old

EDIT: Sam corrected my understanding

Change orthogonal to skew) ~~Fail with error~~ Silently does the wrong thing -- AAAHHH!

No, there it fails 😎

jan-janssen · 2021-01-19T09:41:03Z

The failing unit tests are addressed in #28

liamhuber · 2021-01-19T09:50:52Z

This starts to get fundamental. This pull request addresses one part of the issue above:

My argument is that these parts are not necessarily separable issues.

(A,B):Fails silently, (B,A):Fails <-- Bad

(A,B):Fails silently, (B,A):Works <-- much worse

Silent failures should be high priority to squash. Making a reverse operation fully functional only exacerbates the problem and raises the priority. The reverse doesn't need to work, it just needs to fail cleanly.

The tests for this part would require running lammps and I thought we agreed that we do not want to run simulation codes in the regular unit tests.

Agreed, we should probably avoid this in the regular unit tests, which is why I'm absolutely not going to insist such a test be part of this PR. However, this PR definitely highlights the need for tests, be they unit or integration.

jan-janssen · 2021-01-19T10:06:47Z

If we do not fix this the calculation of elastic constants using the old class from Yury does not work in interactive mode. But this class is already used in current high through projects like http://atomistictools.org . This functionality was broken when we moved away from having triclinic cells as the default. I am against dropping existing functionality in favour of abstract ideals.

samwaseda · 2021-01-19T10:20:09Z

If we do not fix this the calculation of elastic constants using the old class from Yury does not work in interactive mode. But this class is already used in current high through projects like http://atomistictools.org . This functionality was broken when we moved away from having triclinic cells as the default. I am against dropping existing functionality in favour of abstract ideals.

It should still work if the job is static.
Whether it works or not in the interactive mode depends also on the order of execution: if the orthogonal box doesn't happen to be the very first run, then it returns wrong results silently, which is even worse than now.

And I wouldn't call it an "abstract ideal". It's a real problem.

Update from master

jan-janssen · 2021-01-19T10:32:30Z

So then the solution is to reset xy, xz and yz to 0.0 and afterwards change the box to orthogonal?

liamhuber · 2021-01-19T11:15:32Z

So then the solution is to reset xy, xz and yz to 0.0 and afterwards change the box to orthogonal?

Shouldn't they be none for orthogonal?

jan-janssen · 2021-01-19T11:55:41Z

Shouldn't they be none for orthogonal?

I do not think LAMMPS supports none here that is why afterwards I change the cell to orthogonal.

samwaseda · 2021-01-19T17:34:18Z

I guess the first thing we can try is to set change_box orthogonal as you did in one of the commits and hope that LAMMPS complains if we go from triclinic to orthogonal. If it doesn't complain, we probably have to find a different solution...

liamhuber · 2021-01-19T18:11:44Z

I guess the first thing we can try is to set change_box orthogonal as you did in one of the commits and hope that LAMMPS complains if we go from triclinic to orthogonal. If it doesn't complain, we probably have to find a different solution...

The Lammps-crash error message is a little cryptic, but if this does manage to crash things I'd put this in the "good enough" category.

Since the check needs to be performed anyway, it would be lovely to have it codified as a tiny integration test.

jan-janssen · 2021-01-19T19:04:58Z

I guess the first thing we can try is to set change_box orthogonal as you did in one of the commits and hope that LAMMPS complains if we go from triclinic to orthogonal. If it doesn't complain, we probably have to find a different solution...

That is what is used in the current version, first I resent xy, xz and yz to 0.0 and then I switch to ortho

liamhuber · 2021-01-19T19:15:28Z

Then is Lammps crashing on the reverse transformation as hoped? If not some further change to force the crash (either in pyiron or just directly in lammps) is still needed

jan-janssen · 2021-01-19T19:22:10Z

Then is Lammps crashing on the reverse transformation as hoped? If not some further change to force the crash (either in pyiron or just directly in lammps) is still needed

No, also the second part is now fixed.

liamhuber

I won't insist on it, but I think it would be excellent if you included whatever code you're currently using to verify the forward and backward transitions as an integration test (might need to make a new folder for it, but can follow the same format as contrib; also no immediate need to get integration in the ci as even having it there for manual running would be super helpful IMO)

samwaseda · 2021-01-19T19:56:05Z

That is what is used in the current version, first I resent xy, xz and yz to 0.0 and then I switch to ortho

Ah now I get it. I also thought about this possibility, but wasn’t sure if the performance is not badly affected by it. The question is also whether it really makes sense to use both triclinic and orthogonal, or we simply use only triclinic with all shear components equal to zero. Anyway, for now I would accept the solution you suggested (although again I think we should do a performance test).

Lammps: Use change_box when switching from orthogonal box to triclini…

16f8d3c

…c box

jan-janssen requested review from liamhuber and samwaseda January 18, 2021 16:11

samwaseda reviewed Jan 18, 2021

View reviewed changes

samwaseda requested changes Jan 18, 2021

View reviewed changes

liamhuber reviewed Jan 18, 2021

View reviewed changes

remove triclinic from second change box

c71e865

jan-janssen commented Jan 19, 2021

View reviewed changes

jan-janssen force-pushed the lammps_triclinic branch from f22d6a8 to c71e865 Compare January 19, 2021 08:09

liamhuber self-requested a review January 19, 2021 08:53

liamhuber requested changes Jan 19, 2021

View reviewed changes

jan-janssen and others added 2 commits January 19, 2021 11:21

Merge pull request #30 from pyiron/master

aed0dbe

Update from master

another try

04ad909

Update test

72e9fae

fix spaces

3468bd0

liamhuber approved these changes Jan 19, 2021

View reviewed changes

liamhuber approved these changes Jan 20, 2021

View reviewed changes

jan-janssen merged commit bc4c321 into master Jan 22, 2021

delete-merged-branch bot deleted the lammps_triclinic branch January 22, 2021 16:57

		@@ -114,12 +114,18 @@ def interactive_cells_setter(self, cell):
		warnings.warn('set_relative() is deprecated as of 2020-02-26. It is not guaranteed from pyiron_atomistics vers. 0.3')

Lammps: Use change_box when switching from orthogonal box to triclini… #25

Lammps: Use change_box when switching from orthogonal box to triclini… #25

Conversation

jan-janssen commented Jan 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 18, 2021 • edited

Pull Request Test Coverage Report for Build 495814342

💛 - Coveralls

samwaseda left a comment

Choose a reason for hiding this comment

jan-janssen commented Jan 18, 2021 • edited

liamhuber left a comment

Choose a reason for hiding this comment

samwaseda commented Jan 18, 2021

jan-janssen commented Jan 18, 2021

samwaseda commented Jan 18, 2021

jan-janssen commented Jan 18, 2021

samwaseda commented Jan 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samwaseda Jan 19, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamhuber left a comment • edited

Choose a reason for hiding this comment

Old

New (unacceptable)

Requested

samwaseda commented Jan 19, 2021

Old

jan-janssen commented Jan 19, 2021

Old

New

samwaseda commented Jan 19, 2021

Old

jan-janssen commented Jan 19, 2021

liamhuber commented Jan 19, 2021

jan-janssen commented Jan 19, 2021

samwaseda commented Jan 19, 2021

jan-janssen commented Jan 19, 2021

liamhuber commented Jan 19, 2021

jan-janssen commented Jan 19, 2021

samwaseda commented Jan 19, 2021

liamhuber commented Jan 19, 2021

jan-janssen commented Jan 19, 2021

liamhuber commented Jan 19, 2021

jan-janssen commented Jan 19, 2021

liamhuber left a comment

Choose a reason for hiding this comment

samwaseda commented Jan 19, 2021

coveralls commented Jan 18, 2021 •

edited

jan-janssen commented Jan 18, 2021 •

edited

samwaseda Jan 19, 2021 •

edited

liamhuber left a comment •

edited