Navigation Menu

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

New hoomd example #14

Merged
merged 22 commits into from Nov 29, 2020
Merged

New hoomd example #14

merged 22 commits into from Nov 29, 2020

Conversation

atravitz
Copy link
Contributor

For a while we've been needing to update some of these examples. Here's a proposed update to the HOOMD-blue example. The most notable change is the addition of a walk-through readme.

@atravitz atravitz requested review from a team as code owners July 27, 2020 19:36
@atravitz atravitz requested review from b-butler, kidrahahjo and pepak13 and removed request for a team, b-butler and kidrahahjo July 27, 2020 19:36
@mikemhenry
Copy link
Contributor

I really like this example!

@mikemhenry mikemhenry self-requested a review August 6, 2020 16:24
Copy link
Contributor

@mikemhenry mikemhenry left a comment

Choose a reason for hiding this comment

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

Okay so the only other thing that might be worth adding, is to store the run_steps in the job document, that way we could introduce the concept that if you need to run the simulation longer, you just need to bump run_steps in the job doc. Maybe that is for a different tutorial, but it is one of the first things that people run into, they want to run their simulation longer since it hasn't equilibrated yet, but if they want to run it longer, they run into issues since it will mess up the state points.

projects/flow.hoomd.lj/src/README.md Outdated Show resolved Hide resolved
projects/flow.hoomd.lj/src/README.md Outdated Show resolved Hide resolved
projects/flow.hoomd.lj/src/init.py Show resolved Hide resolved
@atravitz
Copy link
Contributor Author

Okay so the only other thing that might be worth adding, is to store the run_steps in the job document, that way we could introduce the concept that if you need to run the simulation longer, you just need to bump run_steps in the job doc. Maybe that is for a different tutorial, but it is one of the first things that people run into, they want to run their simulation longer since it hasn't equilibrated yet, but if they want to run it longer, they run into issues since it will mess up the state points.

I agree that it's good to encourage this as a default - I'll change it and add a note as to why we store it in the job doc.

@csadorf
Copy link
Contributor

csadorf commented Sep 28, 2020

@atravitz I believe this is ready for another round of review? If yes, could you request that?

Besides that, it appears that there are some build issues, any idea what might be going on?

Copy link
Member

@bdice bdice left a comment

Choose a reason for hiding this comment

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

@atravitz The style of the README has to follow a particular format. The continuous integration I added in #17 (and which used to be enabled long ago) parses the README and runs the commands it finds in code blocks beneath the heading # Usage. The updated text is definitely useful but the list formatting will need to look more like the previous. If you want help or an example, let me know.

Also should status.txt be removed (and added to .gitignore)?

@atravitz
Copy link
Contributor Author

atravitz commented Nov 9, 2020

@atravitz The style of the README has to follow a particular format. The continuous integration I added in #17 (and which used to be enabled long ago) parses the README and runs the commands it finds in code blocks beneath the heading # Usage. The updated text is definitely useful but the list formatting will need to look more like the previous. If you want help or an example, let me know.

Also should status.txt be removed (and added to .gitignore)?

Would breaking the commands into code blocks be sufficient? Should we have print_status.sh in there at all?

@bdice
Copy link
Member

bdice commented Nov 9, 2020

Would breaking the commands into code blocks be sufficient? Should we have print_status.sh in there at all?

@atravitz I think so. You can refer to the other examples. The CI will run python flow-test.py flow.hoomd.lj from the projects/ directory, and you can also run that locally to test it.

I don't think we need print_status.sh, you can remove it along with status.txt.

@atravitz
Copy link
Contributor Author

Would breaking the commands into code blocks be sufficient? Should we have print_status.sh in there at all?

@atravitz I think so. You can refer to the other examples. The CI will run python flow-test.py flow.hoomd.lj from the projects/ directory, and you can also run that locally to test it.

I don't think we need print_status.sh, you can remove it along with status.txt.

@bdice let me know if this looks good to you

Copy link
Member

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Thanks for the revisions @atravitz. I have some suggestions below.

projects/flow.hoomd.lj/README.md Outdated Show resolved Hide resolved
projects/flow.hoomd.lj/README.md Outdated Show resolved Hide resolved
projects/flow.hoomd.lj/README.md Outdated Show resolved Hide resolved
projects/flow.hoomd.lj/README.md Outdated Show resolved Hide resolved
projects/flow.hoomd.lj/README.md Outdated Show resolved Hide resolved
projects/flow.hoomd.lj/README.md Outdated Show resolved Hide resolved
projects/flow.hoomd.lj/README.md Outdated Show resolved Hide resolved
projects/flow.hoomd.lj/README.md Outdated Show resolved Hide resolved

@MyProject.operation
@MyProject.pre(initialized)
@MyProject.pre.isfile("init.gsd")
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to use after?

Suggested change
@MyProject.pre.isfile("init.gsd")
@MyProject.pre.after(initialize)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally I like using isfile here because it seems like the most explicit way of showing how pre/post conditions work.

Co-authored-by: Bradley Dice <bdice@bradleydice.com>
Copy link
Member

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks @atravitz.

@bdice
Copy link
Member

bdice commented Nov 29, 2020

I'm going to go ahead and merge this. The PR been stalled for a long time and it is a significant improvement over the existing tutorial.

@bdice bdice merged commit 6ef7170 into master Nov 29, 2020
@bdice bdice deleted the new_hoomd_example branch November 29, 2020 22:52
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.

None yet

4 participants