Skip to content
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

[2] Coord gen new #29

Merged
merged 27 commits into from
Jun 8, 2020
Merged

[2] Coord gen new #29

merged 27 commits into from
Jun 8, 2020

Conversation

fgrunewald
Copy link
Member

Hi,

This is part two of the coordinate generator namely the actual coord generator.

Enjoy!
Fabian

@fgrunewald fgrunewald requested a review from pckroon April 28, 2020 12:14
@fgrunewald fgrunewald added this to In progress in prepare for first release via automation Apr 28, 2020
@fgrunewald fgrunewald changed the title Coord gen new [2] Coord gen new May 19, 2020
Copy link
Member

@pckroon pckroon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm mostly missing docstrings
I still need to look at the tests

polyply/src/backmap.py Outdated Show resolved Hide resolved
polyply/src/backmap.py Outdated Show resolved Hide resolved
polyply/src/gen_coords.py Show resolved Hide resolved
polyply/src/gen_coords.py Outdated Show resolved Hide resolved
polyply/src/generate_templates.py Outdated Show resolved Hide resolved
polyply/src/topology.py Outdated Show resolved Hide resolved
polyply/src/topology.py Outdated Show resolved Hide resolved
polyply/tests/test_backmap.py Outdated Show resolved Hide resolved
polyply/tests/test_backmap.py Outdated Show resolved Hide resolved
Comment on lines 26 to 29
from polyply.src.generate_templates import (_atoms_in_node, find_atoms,
_expand_inital_coords,
compute_volume, map_from_CoG,
extract_block, GenerateTemplates)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from ... import (
    _atoms_in_node, ...
)

prepare for first release automation moved this from In progress to Review in progress May 22, 2020
@fgrunewald fgrunewald requested a review from pckroon June 1, 2020 12:15
Copy link
Member

@pckroon pckroon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also run and appease pylint

polyply/src/backmap.py Outdated Show resolved Hide resolved
polyply/src/gen_coords.py Outdated Show resolved Hide resolved
polyply/src/generate_templates.py Outdated Show resolved Hide resolved
polyply/src/generate_templates.py Outdated Show resolved Hide resolved
polyply/src/generate_templates.py Outdated Show resolved Hide resolved
polyply/tests/test_generate_templates.py Outdated Show resolved Hide resolved
polyply/tests/test_backmap.py Outdated Show resolved Hide resolved
polyply/tests/test_topology.py Outdated Show resolved Hide resolved
polyply/tests/test_topology.py Outdated Show resolved Hide resolved
polyply/tests/test_vs_builder.py Outdated Show resolved Hide resolved
@fgrunewald fgrunewald requested a review from pckroon June 5, 2020 14:29
Copy link
Member

@pckroon pckroon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few more tiny nitpicks

polyply/src/generate_templates.py Show resolved Hide resolved
polyply/src/minimizer.py Outdated Show resolved Hide resolved
del interaction.parameters[-1]
for param in values:
interaction.parameters.append(param)
interaction.parameters[:] = new_parameters[:]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This requires interaction.parameters to always be a list (and not e.g. a tuple). I'm not sure we can make that guarantee.
You can use interaction._replace [1] to create a new interaction with the parameters replaced. You'd then still need to replace the interaction with the new one, but that is guaranteed to be a list.

[1] https://docs.python.org/2/library/collections.html#collections.somenamedtuple._replace

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can. It is for sure a list as it comes out of the topology parser and the namedtuple clearly is setting the attribute as list. So it would be problematic when a programmer uses the function in their own code assuming it can be something else than a list. Although this can happen I think the chance is fairly small here. Let's put this up as an issue for enhancement, because using the replace doesn't propagate as easily all the way back. You need to replace and delete all old interaction lists and then all blocks in the force-field. I wish the named tuple had a replace function.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tuples are immutable, so that makes sense.
What I meant is that Molecule.interactions is guaranteed a dict[str, list]. So you could do something like my_block.interactions[idx] = interaction._replace(parameters=new_parameters). But it's also fine to leave this until it breaks. Be sure to test this behaviour though!

Comment on lines +189 to +191
comb_funcs = {1.0: lorentz_berthelot_rule,
2.0: geometric_rule,
3.0: lorentz_berthelot_rule}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't know why these are floats.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the gromacs default section allows the combination rule number to either be a float or an int. I don't want to type cast them as int in the parser on the one hand as this does not adhere to what is actually written in the file and on the other hand it saves some lines in the parser. I put a comment though.

polyply/src/virtual_site_builder.py Outdated Show resolved Hide resolved
prepare for first release automation moved this from Review in progress to Reviewer approved Jun 8, 2020
@fgrunewald fgrunewald merged commit c7515fc into master Jun 8, 2020
prepare for first release automation moved this from Reviewer approved to Done Jun 8, 2020
@fgrunewald fgrunewald mentioned this pull request Oct 14, 2021
MDSYN2019 pushed a commit to MDSYN2019/polyply_1.0 that referenced this pull request May 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
2 participants