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

doc(examples): 01. Quickstart Jupyter Notebook #95

Merged
merged 2 commits into from
Oct 21, 2019

Conversation

oktak
Copy link
Contributor

@oktak oktak commented Oct 11, 2019

#93
Added Quickstart notebook.

@whitead
Copy link
Collaborator

whitead commented Oct 12, 2019

Thank you for the contribution! We'll have it reviewed shortly.

@whitead
Copy link
Collaborator

whitead commented Oct 13, 2019

Looks good, a few small changes:

  1. With the simple compiling instructions, you shouldn't need to do the complex conda instructions even if you are in a separate conda environment. Those instructions are more for if you have lots of conflicting python versions installed, like on a shared compute cluster. I'll add some text to the README about this and let's just refer to the simple instructions for your notebook.
  2. Please remove the code amended. That's good for me to know, but probably not someone trying out hoomd-tf. You're correct that the graph.save order was wrong, thank you!

@oktak
Copy link
Contributor Author

oktak commented Oct 14, 2019

Looks good, a few small changes:

1. With the simple compiling instructions, you shouldn't need to do the complex conda instructions even if you are in a separate conda environment. Those instructions are more for if you have lots of conflicting python versions installed, like on a shared compute cluster. I'll add some text to the README about this and let's just refer to the simple instructions for your notebook.

2. Please remove the code amended. That's good for me to know, but probably not someone trying out hoomd-tf.  You're correct that the `graph.save` order was wrong, thank you!

Thanks for your reply. So your suggestion is to remove the texts, and leave the title and those code blocks. Am I correct?

  1. It's because I want to keep my OS's python environment clean. And I am not sure if the others build the code without conda. So I've chosen to mention it.

  2. It's ok for me to remove that. But I think I should add the inline comment to hoomd.context.initialize("--mode=cpu") line, because when I call that without any parameters, there will be errors that saying '-f' parameter is unexpected. Therefore, I suspect that the coder should choose mode GPU or CPU explicitly.

@whitead
Copy link
Collaborator

whitead commented Oct 16, 2019

Great, thanks. Sure leave the conda instructions you mentioned if you found that using the simple instructions caused issues. 2. Ok please leave the initialization flag in then.

@oktak
Copy link
Contributor Author

oktak commented Oct 17, 2019

Okay, I updated and pushed another commit.

Copy link
Contributor

@dilnoza92 dilnoza92 left a comment

Choose a reason for hiding this comment

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

I also think you need add a section to the README saying that the jupyter command that you will be using also agrees with the python version that has hoomd-tf and tensorflow installed. Maybe it is intuitive but it might be unintuitive for some novice users.

@whitead
Copy link
Collaborator

whitead commented Oct 20, 2019

@dilnoza92 adding running instructions to the readme sounds like a good idea, but maybe that could be separate from this PR.

@dilnoza92
Copy link
Contributor

@whitead yes it can be a new PR.

@whitead whitead merged commit 6b8c1cc into ur-whitelab:master Oct 21, 2019
@whitead whitead mentioned this pull request Oct 21, 2019
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

3 participants