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

Virtual sites in alchemical factory and add ions in WaterBox #259

Merged
merged 3 commits into from
Jul 28, 2017

Conversation

andrrizzi
Copy link
Contributor

Ready for review!

  • Fix AbsoluteAlchemicalFactory does not support virtual sites #258 : add support for virtual sites in AbsoluteAlchemicalFactory.
  • Add positive_ion, negative_ion and ionic_strength constructor arguments to WaterBox. This was necessary to create a TIP4P test with something that could be alchemically modified (the ions).

@jchodera
Copy link
Member

Awesome! This is much more future-proof! A quick read through looks good to me, but might be good to have @Lnaden read through in detail!

@Lnaden
Copy link
Contributor

Lnaden commented Jul 28, 2017

I'll take a dive through it once I am done writing up the part of the website update I'm finishing off

Copy link
Contributor

@Lnaden Lnaden left a comment

Choose a reason for hiding this comment

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

Looks good, logic on the adding and removing forces should be fine since the _alchemically_modify_X functions only create copies.

My only question though, what was wrong about using the getVirtualSite method of the System object to copy over the virtual site?

@andrrizzi
Copy link
Contributor Author

what was wrong about using the getVirtualSite method of the System object to copy over the virtual site?

I tried that first. The VirtualSite SWIG object you obtain by reference_system.getVirtualSite() belongs to the reference System and can't be assigned to others. You'd need to copy it before calling alchemical_system.setVirtualSite(), but OpenMM failed when I tried to deepcopy it. I don't think it's implemented.

@Lnaden
Copy link
Contributor

Lnaden commented Jul 28, 2017

Ah, that makes sense then. Its probably coded similar to the integrator then once it gets assigned a context, its consumed for the purposes of copying to use elsewhere. Looks good otherwise

@andrrizzi andrrizzi merged commit 47c3a24 into master Jul 28, 2017
@andrrizzi andrrizzi deleted the virtual-sites branch July 28, 2017 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants