-
Notifications
You must be signed in to change notification settings - Fork 677
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 a reader for TNG files using pytng #3765
Conversation
Hello @hmacdope! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2022-09-09 18:03:04 UTC |
Does pytng have conda packages already? |
|
@IAlibay I think your review comments have been addressed here |
Thanks, unless this has a time sensitive component (please let me know if that is the case), I'll re-review later this week (after the 13th). |
Sorry about the lack of review here, this is still on my radar the week has just been a bit busy. I'll try to go over it tomorrow. |
Any chance of a final review here @MDAnalysis/coredevs? :) |
Yeah sorry I did note it down earlier today when I was looking at what PRs needed merging. I'll try to re-review over the weekend. |
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.
Sorry about the very long time back to review this. Just a couple of tiny things.
I'm concurrently updating CI on pytng just to make sure upstream is all good when we do a release.
try: | ||
import pytng | ||
except ImportError: | ||
#: Indicates whether pytng is found. |
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.
readthedocs is having a field day here, can you confirm this renders fine locally @hmacdope ? https://mdanalysis--3765.org.readthedocs.build/en/3765/documentation_pages/coordinates/TNG.html
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.
Seems to be ok locally, what specifically where you looking at?
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.
This seems out of place https://mdanalysis--3765.org.readthedocs.build/en/3765/documentation_pages/coordinates/TNG.html#MDAnalysis.coordinates.TNG.HAS_PYTNG
There's also some weird placement of the author list.
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.
Removing the comments of the form #:
fixed it.
We can ignore most of the linter stuff, but do have a look at the output if there's anything you think is worth it. Specifically the |
@IAlibay I think i have addressed everything including formatting. :) |
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.
lgtm!
This has been years in the making. Thank you for making this milestone happen! |
Well done, everyone! |
Fixes #3237 and partially addresses #865 (reading TNG)
This PR adds functionality for reading TNG files using pytng (finally). 😄
The limitations of reading a TNG file using pytng are also clearly outlined in the docs.
Changes made in this Pull Request:
TODO
Need to add tests using an example data file
PR Checklist