-
Notifications
You must be signed in to change notification settings - Fork 646
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
Partial Fix: Change order of arguments for adding auxiliary data for 3.0 release (Issue #3811) #4532
base: develop
Are you sure you want to change the base?
Conversation
…, due to pytests without auxdata
Hello @talagayev! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2024-03-29 21:57:00 UTC |
Linter Bot Results:Hi @talagayev! Thanks for making this PR. We linted your code and found the following: Some issues were found with the formatting of your code.
Please have a look at the Please note: The |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #4532 +/- ##
===========================================
- Coverage 93.65% 93.63% -0.03%
===========================================
Files 168 180 +12
Lines 21215 22294 +1079
Branches 3908 3908
===========================================
+ Hits 19869 20875 +1006
- Misses 888 961 +73
Partials 458 458 ☔ View full report in Codecov by Sentry. |
I checked again which tests would fail if with the error mesage being:
all three of the errors would rely on the missing of the required |
Quick follow-up on the reason of the errors during The error in them stands from
which expects an But gets instead an |
@hmacdope you suggested folks work on this issue, however I'm not sure this can be tackled right now, i.e. this looks like a 3.0 breaking change? (might be missing something) |
@IAlibay its not mergeable right now, but we could make the change and leave it open for 3.0 and do another PR for a deprecation warning. Would that be OK? |
:/ I think I understand your plan @hmacdope I'll say this in two parts:
In the case of aux, it's infrequently touched enough that code conflicts might not be an issue - not sure what @BFedder's remaining plans were. |
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 PR looks really good and if we could, I'd merge it. For GSOC purposes I'd count the PR.
@hmacdope if you want to count it, please add an approving review.
As per discussion, this has to go into 3.0.0 so I am blocking it.
@@ -34,6 +34,7 @@ Fixes | |||
* Fix deploy action to use the correct version of the pypi upload action. | |||
|
|||
Enhancements | |||
* Change order of arguments for adding auxiliary data (Issue #3811) |
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 is a Change so it should go there.
Additionally, it's a breaking change (see @IAlibay 's comments) so it won't go into 2.8.0, which is unfortunate.
I'd suggest to make this clear, start a new section
??/??/?? talagayev
* 3.0.0
Changes:
....
@@ -1701,7 +1700,7 @@ def copy(self): | |||
|
|||
new.ts = self.ts.copy() | |||
for auxname, auxread in self._auxs.items(): | |||
new.add_auxiliary(auxname, auxread.copy()) | |||
new.add_auxiliary(auxread.copy(), auxname) |
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.
We'll eventually want this covered by tests. Not sure why it wasn't originally covered.
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.
Ah yes, from what i've seen in the tests there is test_copy_with_auxiliary_pyedr()
in test_copying.py
, which does call the copy
function from the ReaderBase
class, but there is no case of testing the copy
function of the SingleFrameReaderBase
class
@@ -451,7 +451,7 @@ def copy(self): | |||
new[self.ts.frame] | |||
|
|||
for auxname, auxread in self._auxs.items(): | |||
new.add_auxiliary(auxname, auxread.copy()) | |||
new.add_auxiliary(auxread.copy(), auxname) |
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.
tests... not your fault, though
Should it stay right now as it is, with |
Given that this PR has to wait for 3.0, it would make sense to implement the complete change to fully address #3811, i.e., make |
I am not quite sure how we will implement deprecation/change warnings. That's a separate issue/PR, though. |
Btw, @talagayev please also click the checkbox for Developer Certificate of Origin in the issue. Without it, we will not be able to ever merge the contribution. |
Sounds good, will adjust it to make |
Done |
@fiona-naughton I've assigned you to keep an eye on this PR, given that you invented the aux system. We can't merge it until 3.0 so there's not much to do right now, just being aware of it. Thank you! |
More than happy for this to count for GSOC. |
I still hope to get a NumPy reader out at some point, but haven't been able to yet for a variety of reasons. I'll keep these changes here in mind if I do find the time before 3.0. In the meantime, these changes here look good to me. |
Partially Fixes #3811
Partially, since currently i modified the order of
add_auxiliary
, but retainedaux_data
as optional, since some of the pytests, which would have onlyaux_spec
would display errors in pytests and since it is already multiple files, that were modified i thought that it makes sense to first change the order and then make it mandatory if desired.Changes made in this Pull Request:
aux_data
andaux_spec
inpackage/MDAnalysis/coordinates/base.py
.testsuite/MDAnalysisTests/auxiliary
andtestsuite/MDAnalysisTests/coordinates
tests that usedadd_auxiliary
add_auxiliary
inpackage/MDAnalysis/coordinates/base.py
incopy
in classesclass ReaderBase
andclass SingleFrameReaderBase
that both used this functionadd_auxiliary
in the docs ofpackage/MDAnalysis/auxiliary/EDR.py
andpackage/MDAnalysis/coordinates/base.py
I am not sure if these are all of the docs where
add_auxiliary
is used, i checked the package and didn't find any other cases of usage ofadd_auxiliary
in docs, except for the ones that i modifiedPR Checklist
Developers certificate of origin
📚 Documentation preview 📚: https://mdanalysis--4532.org.readthedocs.build/en/4532/