MDAnalysis / panedr

Reads Gromacs EDR file to populate a pandas dataframe
GNU Lesser General Public License v2.1
31 stars 7 forks source link

Make pyedr also return a dictionary of units #56

Closed BFedder closed 2 years ago

BFedder commented 2 years ago

Following the discussion on units on PR 3749, I have changed pyedr here so it returns a dictionary of the units it reads from the EDR file as well.

pep8speaks commented 2 years ago

Hello @BFedder! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:

Comment last updated at 2022-08-31 12:21:59 UTC
codecov[bot] commented 2 years ago

Codecov Report

Base: 89.21% // Head: 89.70% // Increases project coverage by +0.49% :tada:

Coverage data is based on head (8e5f566) compared to base (ccfdc89). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #56 +/- ## ========================================== + Coverage 89.21% 89.70% +0.49% ========================================== Files 7 7 Lines 482 505 +23 ========================================== + Hits 430 453 +23 Misses 52 52 ``` | [Impacted Files](https://codecov.io/gh/MDAnalysis/panedr/pull/56?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MDAnalysis) | Coverage Δ | | |---|---|---| | [panedr/panedr/\_\_init\_\_.py](https://codecov.io/gh/MDAnalysis/panedr/pull/56/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MDAnalysis#diff-cGFuZWRyL3BhbmVkci9fX2luaXRfXy5weQ==) | `100.00% <100.00%> (ø)` | | | [panedr/panedr/panedr.py](https://codecov.io/gh/MDAnalysis/panedr/pull/56/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MDAnalysis#diff-cGFuZWRyL3BhbmVkci9wYW5lZHIucHk=) | `100.00% <100.00%> (ø)` | | | [panedr/panedr/tests/test\_edr.py](https://codecov.io/gh/MDAnalysis/panedr/pull/56/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MDAnalysis#diff-cGFuZWRyL3BhbmVkci90ZXN0cy90ZXN0X2Vkci5weQ==) | `98.91% <100.00%> (+0.08%)` | :arrow_up: | | [pyedr/pyedr/\_\_init\_\_.py](https://codecov.io/gh/MDAnalysis/panedr/pull/56/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MDAnalysis#diff-cHllZHIvcHllZHIvX19pbml0X18ucHk=) | `100.00% <100.00%> (ø)` | | | [pyedr/pyedr/pyedr.py](https://codecov.io/gh/MDAnalysis/panedr/pull/56/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MDAnalysis#diff-cHllZHIvcHllZHIvcHllZHIucHk=) | `82.72% <100.00%> (+0.38%)` | :arrow_up: | | [pyedr/pyedr/tests/datafiles.py](https://codecov.io/gh/MDAnalysis/panedr/pull/56/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MDAnalysis#diff-cHllZHIvcHllZHIvdGVzdHMvZGF0YWZpbGVzLnB5) | `100.00% <100.00%> (ø)` | | | [pyedr/pyedr/tests/test\_edr.py](https://codecov.io/gh/MDAnalysis/panedr/pull/56/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MDAnalysis#diff-cHllZHIvcHllZHIvdGVzdHMvdGVzdF9lZHIucHk=) | `96.39% <100.00%> (+0.20%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MDAnalysis). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MDAnalysis)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

BFedder commented 2 years ago

I have updated this now so the units are also returned through panedr, not just pyedr

BFedder commented 2 years ago

Having looked at the code again to add an example for the readme, I have realised that having the energy dict as part of the return value for edr_to_dict and edr_to_df is problematic, because people would then have a tuple at hand when they might not expect it from the function name. I have instead now written a new function, get_unit_dictionary, that returns the unit dictionary alone.

hmacdope commented 2 years ago

@IAlibay do you have any blocking concerns here? Otherwise I will merge so we can progress on https://github.com/MDAnalysis/mdanalysis/pull/3749

IAlibay commented 2 years ago

I'll re-review later today, MDA release comes as a priority.

hmacdope commented 2 years ago

@IAlibay any outstanding comments or can I merge? :)

IAlibay commented 2 years ago

I'll review in a few

orbeckst commented 2 years ago

Are the settings on this repo that new commits dismiss reviews automatically? Is this desired? @hmacdope @IAlibay @jbarnoud @BFedder

Personally, this looks like generating too much work. I am unlikely to go back to a PR if I approved it because this means (to me at least) that all the things I cared about had been addressed. I suggest to change the settings.

BFedder commented 2 years ago

I don't dismiss the reviews on purpose at least (and I also find that "dismissed stale reviews" sounds a bit harsh), so I hope it happens automatically rather than by some mistake I might make. I agree with you - this should not happen automatically I think. If there are big changes still, a new review could always be requested.

IAlibay commented 2 years ago

@orbeckst that might be me just using default settings when I set up branch protection. This is fixed now. I assume by this that we're ok to merge?