New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pair srp + fix deform triclinic flip yes #17

Closed
timsirk opened this Issue Nov 12, 2015 · 13 comments

Comments

Projects
None yet
2 participants
@timsirk
Collaborator

timsirk commented Nov 12, 2015

Pair srp fails with an error when used with combination of triclinic box + fix deform + flip yes + large strain. This happens due to the order that fix deform and fix srp occur in pre_exchange step. Currently fix srp is automatically invoked and runs ~last in pre_exchange. This patch allows the user to (optionally) define the order of fix srp in the input script. The doc page is also updated.

patch.26Oct15.txt

@akohlmey

This comment has been minimized.

Show comment
Hide comment
@akohlmey

akohlmey Nov 13, 2015

Member

Been thinking about this for a bit, and not very happy with the proposed change. It makes the interface quite inconsistent. Either one should always have to define fix srp or it should always be automatically defined. As for the latter, i tried moving the fix creation to the constructor and unlike what is indicated by the comments in the source, it seems to be working fine, including restarting.

Member

akohlmey commented Nov 13, 2015

Been thinking about this for a bit, and not very happy with the proposed change. It makes the interface quite inconsistent. Either one should always have to define fix srp or it should always be automatically defined. As for the latter, i tried moving the fix creation to the constructor and unlike what is indicated by the comments in the source, it seems to be working fine, including restarting.

@akohlmey

This comment has been minimized.

Show comment
Hide comment
@akohlmey

akohlmey Nov 13, 2015

Member

Please try out the code in commit 7bf781c instead.

Member

akohlmey commented Nov 13, 2015

Please try out the code in commit 7bf781c instead.

@akohlmey akohlmey self-assigned this Nov 13, 2015

@akohlmey

This comment has been minimized.

Show comment
Hide comment
@akohlmey

akohlmey Nov 17, 2015

Member

Tim,
any updates on this issue? If not, i'll close it as resolved.

Member

akohlmey commented Nov 17, 2015

Tim,
any updates on this issue? If not, i'll close it as resolved.

@timsirk

This comment has been minimized.

Show comment
Hide comment
@timsirk

timsirk Nov 17, 2015

Collaborator

one more day please, I haven't been able to test it yet.

Collaborator

timsirk commented Nov 17, 2015

one more day please, I haven't been able to test it yet.

@timsirk

This comment has been minimized.

Show comment
Hide comment
@timsirk

timsirk Nov 18, 2015

Collaborator

Just tested - this change (by itself) only allows restarts to be read if they were written between runs, and not during a run. The core issue is that fix_srp init tries to check if the user reserved an extra unused atom type for the bond particles. This check is fine when reading a datafile or end-of-run restart file, because those only contain the true number of atoms, and not these exotic bond particles. however during-the-run restart files have the bond particle coordinates stored as per-atom data. the check uses the fix->restart_reset flag to decide if the user forgot the extra atom type, or if its legitimate coordinates from a during-the-run restart.

It looks like fix->restart_reset isn't set during the check if add_fix() is called in the constructor. So in the current form the check will always fail for during-the-run restarts. I do agree it would be a better solution to add the fix in constructor if possible.

Collaborator

timsirk commented Nov 18, 2015

Just tested - this change (by itself) only allows restarts to be read if they were written between runs, and not during a run. The core issue is that fix_srp init tries to check if the user reserved an extra unused atom type for the bond particles. This check is fine when reading a datafile or end-of-run restart file, because those only contain the true number of atoms, and not these exotic bond particles. however during-the-run restart files have the bond particle coordinates stored as per-atom data. the check uses the fix->restart_reset flag to decide if the user forgot the extra atom type, or if its legitimate coordinates from a during-the-run restart.

It looks like fix->restart_reset isn't set during the check if add_fix() is called in the constructor. So in the current form the check will always fail for during-the-run restarts. I do agree it would be a better solution to add the fix in constructor if possible.

@akohlmey

This comment has been minimized.

Show comment
Hide comment
@akohlmey

akohlmey Nov 19, 2015

Member

Wouldn't it be simpler and cleaner to just generally delete all atoms of type bptype at the beginning of a run and then create the mid-bond particles for that type? That would make the whole setup process independent from what is stored in a restart or data file (or trajectory file). There could be a warning that says "Deleting X atoms of type Y. Replacing with Z mid-bond particles." If somebody points bptype at a type with atoms to be kept is is a user error. I always dislike to make behavior of a program dependent on a particular process.

I will look at how this could be done.

Member

akohlmey commented Nov 19, 2015

Wouldn't it be simpler and cleaner to just generally delete all atoms of type bptype at the beginning of a run and then create the mid-bond particles for that type? That would make the whole setup process independent from what is stored in a restart or data file (or trajectory file). There could be a warning that says "Deleting X atoms of type Y. Replacing with Z mid-bond particles." If somebody points bptype at a type with atoms to be kept is is a user error. I always dislike to make behavior of a program dependent on a particular process.

I will look at how this could be done.

@akohlmey

This comment has been minimized.

Show comment
Hide comment
@akohlmey

akohlmey Nov 19, 2015

Member

Oh, and i should probably also add a check to verify, if the fix actually is the first in the list, that is run at the required stage.

Member

akohlmey commented Nov 19, 2015

Oh, and i should probably also add a check to verify, if the fix actually is the first in the list, that is run at the required stage.

@timsirk

This comment has been minimized.

Show comment
Hide comment
@timsirk

timsirk Nov 19, 2015

Collaborator

Yes, deleting / reinserting the bond particles at the beginning of the run should work for any case. That seems robust.

Collaborator

timsirk commented Nov 19, 2015

Yes, deleting / reinserting the bond particles at the beginning of the run should work for any case. That seems robust.

@timsirk

This comment has been minimized.

Show comment
Hide comment
@timsirk

timsirk Nov 19, 2015

Collaborator

Thanks for looking into this. The warning seems sufficient. And the hard stop checkpoint more trouble than it was worth.

Collaborator

timsirk commented Nov 19, 2015

Thanks for looking into this. The warning seems sufficient. And the hard stop checkpoint more trouble than it was worth.

@akohlmey

This comment has been minimized.

Show comment
Hide comment
@akohlmey

akohlmey Dec 2, 2015

Member

Tim, please check out the updated code for fix/pair_srp.cpp/h below. This allows to set bptype in the srp pair style directly, and deletes any excess particles particles of bptype before creating new ones. it can start from mid-run and between run restarts and data files converted from either. so everything seems to be working, but the energies are off. i must be missing something, but i am running out of ideas.

https://www.dropbox.com/s/kw79d5rdvyydeno/lammps-srp-refactor.tar.gz?dl=0

Member

akohlmey commented Dec 2, 2015

Tim, please check out the updated code for fix/pair_srp.cpp/h below. This allows to set bptype in the srp pair style directly, and deletes any excess particles particles of bptype before creating new ones. it can start from mid-run and between run restarts and data files converted from either. so everything seems to be working, but the energies are off. i must be missing something, but i am running out of ideas.

https://www.dropbox.com/s/kw79d5rdvyydeno/lammps-srp-refactor.tar.gz?dl=0

@timsirk

This comment has been minimized.

Show comment
Hide comment
@timsirk

timsirk Dec 8, 2015

Collaborator

Pleas have a look at this patch. Your update was very close to working. The strategy for restarts is good and works well. The energy mismatch was due to the union ubuf in fix_srp. It wasn't giving a readable value. I don't understand aliasing well enough to really get to the bottom of it. The update uses a static cast.

patch.srp.txt

Collaborator

timsirk commented Dec 8, 2015

Pleas have a look at this patch. Your update was very close to working. The strategy for restarts is good and works well. The energy mismatch was due to the union ubuf in fix_srp. It wasn't giving a readable value. I don't understand aliasing well enough to really get to the bottom of it. The update uses a static cast.

patch.srp.txt

@akohlmey

This comment has been minimized.

Show comment
Hide comment
@akohlmey

akohlmey Dec 8, 2015

Member

thanks a lot. using a cast is fine for the time being, i'll just insert a (runtime) warning noting that pair srp is not fully compatible with 64-bit tags. if that specific part of the code is the problem, i can look into fixing it at a later point. the changes i made were the way they are "supposed to be(tm)", but something must have been overlooked.

Member

akohlmey commented Dec 8, 2015

thanks a lot. using a cast is fine for the time being, i'll just insert a (runtime) warning noting that pair srp is not fully compatible with 64-bit tags. if that specific part of the code is the problem, i can look into fixing it at a later point. the changes i made were the way they are "supposed to be(tm)", but something must have been overlooked.

@akohlmey

This comment has been minimized.

Show comment
Hide comment
@akohlmey

akohlmey Dec 8, 2015

Member

the combined changes are now merged into LAMMPS-ICMS in commit 8446b8c and sent to steve for inclusion into upstream. i'll open a new ticket for making srp fully compatible with 64-bit tags or adding a warning.

Member

akohlmey commented Dec 8, 2015

the combined changes are now merged into LAMMPS-ICMS in commit 8446b8c and sent to steve for inclusion into upstream. i'll open a new ticket for making srp fully compatible with 64-bit tags or adding a warning.

@akohlmey akohlmey closed this Dec 8, 2015

rbberger pushed a commit to rbberger/lammps that referenced this issue Sep 8, 2016

Merge pull request #17 from akohlmey/small-doc-fixes
corrections for various compute something/chunk examples

sjplimp pushed a commit that referenced this issue Jun 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment