search

mosdef-hub / mosdef-workflows

Sample molecular simulation workflows using a MoSDeF and community tools
14 stars 23 forks source link

Update HOOMD LJ example to v3 #31

Closed joaander closed 2 years ago

joaander commented 3 years ago

This PR requires https://github.com/mosdef-hub/mbuild/pull/871

review-notebook-app[bot] commented 3 years ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

joaander commented 3 years ago

This is read to merge once https://github.com/mosdef-hub/mbuild/pull/871 is merged and there are new mbuild and HOOMD releases.

joaander commented 2 years ago

LGTM. The new hoomd3 schema is excellent. I was using this example to play around with mosdef-hub/mbuild#871

One note I have is it would be cool to add name, version, and combining rule to the ff.xml in this example to silence these warnings:

/Users/jenny/miniconda3/envs/mbuild-dev/lib/python3.8/site-packages/foyer/forcefield.py:615: UserWarning: No force field version number found in force field XML file.
  warnings.warn(
/Users/jenny/miniconda3/envs/mbuild-dev/lib/python3.8/site-packages/foyer/forcefield.py:627: UserWarning: No force field name found in force field XML file.
  warnings.warn(
/Users/jenny/miniconda3/envs/mbuild-dev/lib/python3.8/site-packages/foyer/forcefield.py:639: UserWarning: No combining rule found in force field XML file.
  warnings.warn(

I am not familiar with the XML file format. Could someone add that?

also is wrap implemented with the snapshot box now too? We could maybe switch that as well.

Yes, Snapshot can wrap particles: https://hoomd-blue.readthedocs.io/en/latest/package-hoomd.html#hoomd.Snapshot.wrap. This is used in mbuild: https://github.com/mosdef-hub/mbuild/blob/a81b4ebbca6b373076d0f63096cea8d91e9081f8/mbuild/formats/hoomd_snapshot.py#L361-L363

joaander commented 2 years ago

@justinGilmer any further changes needed before merging? I'd like to link to this from the HOOMD-blue documentation.

jennyfothergill commented 2 years ago

one change that still might be nice is to replace the <Forcefield > line in the xml with <Forcefield name="Argon-LJ" version="0.0.1" combining_rule="geometric"> to silence the warnings. The name, version, and combining rule I suggest are all just off the top of my head so feel free to change them.

justinGilmer commented 2 years ago

Edits made to the XML, once we are happy with that, we can merge.

justinGilmer commented 2 years ago

@jennyfothergill re-requesting your review as well.

joaander commented 2 years ago

Are these notebooks rendered anywhere? Or is the best case to link directly to the notebook in GitHub?https://github.com/mosdef-hub/mosdef-workflows/blob/master/hoomd_lj/multistate_hoomd_lj.ipynb

jennyfothergill commented 2 years ago

@joaander to my knowledge, no. But they definitely should be! I'll open an issue to add a docs page for this repo. It'd be nice to be able to link to it from mbuild/foyer/gmso docs.

joaander commented 2 years ago

@joaander to my knowledge, no. But they definitely should be! I'll open an issue to add a docs page for this repo. It'd be nice to be able to link to it from mbuild/foyer/gmso docs.

I have had luck with nbsphinx for including notebooks in sphinx docs.

justinGilmer commented 2 years ago

At the moment, we do not have any rendering on a website or elsewhere for these notebooks aside from GitHub's in-browser render. Although I am absolutely ok with us creating a page where we host these and then can link individual examples, etc to an html page. Lets not let this hold up this PR, I can merge this now.

joaander commented 2 years ago

Sorry, I didn't mean to create extra work. I was just asking if there already was one.

justinGilmer commented 2 years ago

Not at all, I think it would be a very nice QOL improvement especially if we want more users to try these out.