-
Notifications
You must be signed in to change notification settings - Fork 48
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
Add with ids #364
Merged
Merged
Add with ids #364
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andrewtarzia
approved these changes
Jul 9, 2021
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like the fact that the tests all passed and the structures were maintained after these changes, means its all good!
src/stk/molecular/functional_groups/functional_groups/generic_functional_group.py
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Requested Reviewers: @andrewtarzia
Note for Reviewers: If you accept the review request add a 👍 to this post
A type error has been discovered!
The solution to this type error is to make the return type of
with_atoms()
to return eitherFunctionalGroup
orGenericFuncitonalGroup
as appropriate, but not the most derived type.However, this is a problem because during construction, we need to make clones, with different atom ids, but we also need to preserve the most derived type, because this information may be used by reaction factories to select an appropriate reaction. For example, you might want a specific bond order between alcohol and carboxylic acid functional groups.
To get around this, I'm adding
with_ids
, it's just likewith_atoms
except it only lets you change atom ids but not atom types. This means that the most derived type can be returned. Internal use ofwith_atoms
has been replaced bywith_ids
so that the most derived type can be preserved.I am also adding
Bond.with_ids
so that you can use the samedict
when usingFunctionalGroup.with_ids
for bonds as well. Otherwise the user would have to create an id map for the functional groups but an atom map for bonds, and that's kind of annoying.Other code changes:
src/stk/molecular/functional_groups/.../functional_group.py:260
: I removed the_with_atoms
method and replaced it with a new implementation ofwith_atoms
. The new implementation does not return a clone of the most derived type, instead it always returns aFunctionalGroup
instance.src/stk/molecular/functional_gorups/.../generic_functional_gorup.py:63
Replaced the_with_atoms
implementation with an implementation ofwith_atoms
, which always returns aGenericFunctionalGroup
instance.src/stk/molecular/molecules/building_block.py:692
: Here I removed the use ofwith_atoms
and did not replace it withwith_ids
.with_atoms
was being used here to minimize memory usage. I.e. the functional groups held internally by the building block held the atoms fromself._atoms
, rather than the atoms the user provided. This means that the atoms which the user created / provided could be garbage collected. Becausewith_atoms
loses the most derived type, it can no longer be used here and there is no way to replace its use here withwith_ids
.src/stk/.../placements_summary/atom_batch.py:45
: I replaced the use of internal lists with internal tuples, just so that the immutability is a bit more clear. The downside is an extra iteration, but immutability is more important in my opinion.