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

AuxReader for EDR Files #3749

Merged
merged 63 commits into from
Sep 19, 2022
Merged
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
63 commits
Select commit Hold shift + click to select a range
f24d296
initial file created, test working on different computers
BFedder Jul 8, 2022
0906225
continued work
BFedder Jul 8, 2022
a7c1a81
Minimum working system: EDRReader
BFedder Jul 9, 2022
485b5d8
Fixed typos, PEP8
BFedder Jul 20, 2022
1048271
added pyedr to CI setup
BFedder Jul 20, 2022
0b49bad
EDRReader now fully functional in stand-alone use
BFedder Jul 20, 2022
e0767b4
Changed EDR so that base.get_description() works
BFedder Jul 20, 2022
0f7b1af
Implemented tests for standalone use
BFedder Jul 24, 2022
4f944b3
Added pyedr to azure-pipelines.yml
BFedder Jul 24, 2022
ca1a014
addressed orbeckst's reviews, fixed test for datafiles.py
BFedder Jul 28, 2022
54022bf
begin refactoring coordinates/base.py
BFedder Jul 29, 2022
a7f27b1
further work on handling energy data
BFedder Jul 29, 2022
ca39385
EDR Reader functionality complete
BFedder Jul 31, 2022
6159f2d
Added pyedr to maintainer/conda/environment.yml
BFedder Aug 3, 2022
f228aa7
PEP8 stuff
BFedder Aug 3, 2022
73143f6
PEP 8
BFedder Aug 3, 2022
3be01c6
improving coverage
BFedder Aug 3, 2022
46584ab
further coverage improvement
BFedder Aug 3, 2022
0733647
added function for returning data from auxreader
BFedder Aug 4, 2022
77d8fa1
modified CHANGELOG
BFedder Aug 4, 2022
3105f0e
Added type hinting
BFedder Aug 4, 2022
b2f30e3
added documentation
BFedder Aug 4, 2022
f48c74c
merged develop into edr_reader
BFedder Aug 4, 2022
1f921ea
Merge branch 'MDAnalysis-develop' into edr_reader
BFedder Aug 4, 2022
9152604
Use List, Dict from typing to fix CI
BFedder Aug 4, 2022
f59e6fa
Update package/MDAnalysis/auxiliary/EDR.py
BFedder Aug 4, 2022
b29747c
Fixed Docstring for EDRStep class
BFedder Aug 4, 2022
3916bdf
Merge branch 'edr_reader' of https://github.com/BFedder/mdanalysis in…
BFedder Aug 4, 2022
0092d34
pep8....
BFedder Aug 4, 2022
9235600
Merge branch 'MDAnalysis:develop' into edr_reader
BFedder Aug 5, 2022
08907c3
addressing some of IAlibay's comments
BFedder Aug 5, 2022
e78ab9e
Moved adding of aux data from coordinates/base to auxiliary/base
BFedder Aug 13, 2022
0119ee1
refined API overhaul and hopefully fixed CI, also now allowing None t…
BFedder Aug 14, 2022
c233970
Addressing reviewer comments
BFedder Aug 15, 2022
cccc1fd
Apply suggestions from code review
BFedder Aug 21, 2022
aa226ca
fixed test for get_data
BFedder Aug 21, 2022
d77517b
changed get_data docstring
BFedder Aug 21, 2022
fed3b06
added class attribute documentation to EDRReader
BFedder Aug 21, 2022
ed20993
added memory usage monitoring
BFedder Aug 21, 2022
cea6a97
Memory warning now reports memory usage
BFedder Aug 22, 2022
3660770
Warning message changed from KB to GB
BFedder Aug 22, 2022
1a0f4e3
fixed parentheses escape by using r-string
BFedder Aug 22, 2022
117d138
updated CHANGELOG, added test for NotImplemented, pickle, etc.
BFedder Aug 24, 2022
c7bde96
removed asterisk option from attach_auxiliary()
BFedder Sep 1, 2022
55a794a
Unit handling for EDRReaders
BFedder Sep 1, 2022
5e7a779
added TODO comments for issue 3811
BFedder Sep 1, 2022
dfaba25
Merge branch 'develop' into edr_reader
BFedder Sep 1, 2022
d1892f6
minor change to trigger GH Actions
BFedder Sep 2, 2022
c5a3a7c
added pyedr to requirements.txt and setup.py for cirrus
BFedder Sep 2, 2022
2cc8390
Merge branch 'develop' into edr_reader
orbeckst Sep 4, 2022
88c4651
Added 'convert_units' kwarg to EDRReader __init__
BFedder Sep 6, 2022
8b71850
Documentation overhaul
BFedder Sep 6, 2022
d599e97
Merge branch 'develop' into edr_reader
BFedder Sep 6, 2022
e8f5998
went from 4810 warnings to 237 in like 7 lines of code
BFedder Sep 6, 2022
336cc80
Merge branch 'edr_reader' of https://github.com/BFedder/mdanalysis in…
BFedder Sep 6, 2022
a62066e
Add test for MDANALYSIS_BASE_UNITS, some doc work
BFedder Sep 6, 2022
0ad0079
revert from ResourceWarning to UserWarning
BFedder Sep 6, 2022
064ab37
pep8 stuff
BFedder Sep 7, 2022
e256498
make pyedr optional and other IAlibay requests
BFedder Sep 12, 2022
8d9e077
PEP8
BFedder Sep 12, 2022
11c3924
changed changelog, docstring; used defaultdict
BFedder Sep 14, 2022
e3a2bb8
fixed CHANGELOG
BFedder Sep 19, 2022
828b2dc
Apply suggestions from code review
BFedder Sep 19, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions .github/actions/setup-deps/action.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,8 @@ inputs:
default: 'pytest-cov<=2.10.1'
pytest-xdist:
default: 'pytest-xdist'
pyedr:
Copy link
Member

Choose a reason for hiding this comment

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

Are we making pyedr an optional dependency? I'm somewhat ok either way, if we could drop the 14MB test files in the next release it's small enough that it could be core if we wanted to. Thoughts @MDAnalysis/coredevs ?

Copy link
Member

Choose a reason for hiding this comment

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

In any case, pyedr needs to go into the setup.py either as an extra_requires (if not a core dependency), or just as an install_requires otherwise

Copy link
Member

Choose a reason for hiding this comment

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

If we can decrease the size to <1 MB then it's just more convenient to have it as a core dependency — it's light weight and we maintain it.

Copy link
Member

Choose a reason for hiding this comment

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

This also needs adding to the azure pipelines file:

python -m pip install --only-binary=h5py
cython
hypothesis
matplotlib
numpy
packaging
pytest
pytest-cov
pytest-xdist
scikit-learn
scipy
h5py>=2.10
tqdm
threadpoolctl
fasteners

Copy link
Member

Choose a reason for hiding this comment

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

We are not at this point making pyedr a core dependency - going by the fact that we aren't touching setup.py etc... (I would like to get more coredev oks on this anyways)

As a result, I would ask that this be moved to the pip optional dependencies section.

default: 'pyedr'
hmacdope marked this conversation as resolved.
Show resolved Hide resolved
# pip-install optional dependencies
duecredit:
default: 'duecredit'
Expand Down Expand Up @@ -132,14 +134,15 @@ runs:
else
conda install ${CONDA_DEPS}
fi

- name: pip_install
shell: bash -l {0}
env:
PIP_MIN_DEPS: |
${{ inputs.coverage }}
${{ inputs.pytest-cov }}
${{ inputs.pytest-xdist }}
${{ inputs.pyedr }}
Copy link
Member

Choose a reason for hiding this comment

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

As above, optional dep please.

PIP_OPT_DEPS: |
${{ inputs.duecredit }}
${{ inputs.parmed }}
Expand All @@ -150,7 +153,6 @@ runs:
else
PIP_DEPS="${PIP_MIN_DEPS} ${{ inputs.extra-pip-deps }}"
fi

# do the install
pip install ${PIP_DEPS}

1 change: 1 addition & 0 deletions azure-pipelines.yml
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ jobs:
tqdm
threadpoolctl
fasteners
pyedr
Copy link
Member

Choose a reason for hiding this comment

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

It's not common practice to have optional deps in azure pipeline runners, I'd remove it for now.

displayName: 'Install dependencies'
# for wheel install testing, we pin to an
# older NumPy, the oldest version we support and that
Expand Down
1 change: 1 addition & 0 deletions maintainer/conda/environment.yml
Original file line number Diff line number Diff line change
Expand Up @@ -33,3 +33,4 @@ dependencies:
- msmb_theme==1.2.0
- sphinx-sitemap==1.0.2
- packaging
- pyedr
BFedder marked this conversation as resolved.
Show resolved Hide resolved
10 changes: 8 additions & 2 deletions package/CHANGELOG
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ The rules for this file:

------------------------------------------------------------------------------
??/??/?? IAlibay, PicoCentauri, orbeckst, hmacdope, rmeli, miss77jun, rzhao271,
yuxuanzhuang, hsadia538
yuxuanzhuang, hsadia538, BFedder

Copy link
Member

Choose a reason for hiding this comment

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

Don't remove this.

* 2.3.0

Expand All @@ -36,6 +36,8 @@ Enhancements
(CZI Performance track, PR #3683)
* Refactor make_downshift_arrays with vector operation in numpy(PR #3724)
* Optimize topology serialization/construction by lazy-building _RA and _SR.
* A new AuxReader for the GROMACS EDR file format was implemented.
Copy link
Member

Choose a reason for hiding this comment

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

Not to be too pedantic but entries in the changelog should be newest first ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, yeah, it says so right there - whoops!

(Issue # 3629, PR # 3749)

BFedder marked this conversation as resolved.
Show resolved Hide resolved
Changes
* To add additional optional packages for different file formats when
Expand All @@ -45,6 +47,10 @@ Changes
as per NEP29 (PR #3737)
* Narrowed variable scope to reduce use of OpenMP `private` clause (PR #3706, PR
#3728)
* `MDAnalysis.coordinates.base.add_auxiliary()` now supports adding of
multiple auxiliary data points at the same time. To this end, an additional
`auxterm` argument is now available. Adding all data found in the auxiliary
data source is possible by passing `auxname="*"`. (PR #3749)

BFedder marked this conversation as resolved.
Show resolved Hide resolved
Deprecations
* The extras_requires target "AMBER" for `pip install ./package[AMBER]`
Expand Down Expand Up @@ -109,7 +115,7 @@ Enhancements
* Added equations for `center_of_mass` and `center_of_geometry`to
documentation (PR #3671)
* Added `center_of_charge` attribute (PR #3671)
* Added `frames` argument to AnalysisBase.run to allow analysis to run on
hmacdope marked this conversation as resolved.
Show resolved Hide resolved
* Added `frames` argument to AnalysisBase.run to allow analysis to run on
arbitrary list of frames (Issue #1985)
* LinearDensity now works with updating AtomGroups (Issue #2508, PR #3617)
* Link PMDA in documentation (PR #3652)
Expand Down
Loading