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

Partial Fix: Change order of arguments for adding auxiliary data for 3.0 release (Issue #3811) #4532

Open
wants to merge 16 commits into
base: develop
Choose a base branch
from

Conversation

talagayev
Copy link
Member

@talagayev talagayev commented Mar 26, 2024

Partially Fixes #3811

Partially, since currently i modified the order of add_auxiliary, but retained aux_data as optional, since some of the pytests, which would have only aux_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:

  • Change order of arguments of aux_data and aux_spec in package/MDAnalysis/coordinates/base.py.
  • Adjusted the Order of those arguments in testsuite/MDAnalysisTests/auxiliary and testsuite/MDAnalysisTests/coordinates tests that used add_auxiliary
  • Changed the order of add_auxiliary in package/MDAnalysis/coordinates/base.py in copy in classes class ReaderBase and class SingleFrameReaderBase that both used this function
  • Changed the order of add_auxiliary in the docs of package/MDAnalysis/auxiliary/EDR.py and package/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 of add_auxiliary in docs, except for the ones that i modified

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?

Developers certificate of origin


📚 Documentation preview 📚: https://mdanalysis--4532.org.readthedocs.build/en/4532/

@pep8speaks
Copy link

pep8speaks commented Mar 26, 2024

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

Line 147:59: W291 trailing whitespace

Line 216:80: E501 line too long (106 > 79 characters)
Line 217:80: E501 line too long (109 > 79 characters)

Comment last updated at 2024-03-29 21:57:00 UTC

Copy link

github-actions bot commented Mar 26, 2024

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.

Code Location Outcome
main package ⚠️ Possible failure
testsuite ⚠️ Possible failure

Please have a look at the darker-main-code and darker-test-code steps here for more details: https://github.com/MDAnalysis/mdanalysis/actions/runs/8438378464/job/23110406304


Please note: The black linter is purely informational, you can safely ignore these outcomes if there are no flake8 failures!

Copy link

codecov bot commented Mar 26, 2024

Codecov Report

Attention: Patch coverage is 33.33333% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 93.63%. Comparing base (f5335b4) to head (6ad9ba0).
Report is 5 commits behind head on develop.

❗ Current head 6ad9ba0 differs from pull request most recent head 0da1436. Consider uploading reports for the commit 0da1436 to get more accurate results

Files Patch % Lines
package/MDAnalysis/coordinates/base.py 50.00% 1 Missing ⚠️
package/MDAnalysis/coordinates/memory.py 0.00% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

@talagayev
Copy link
Member Author

I checked again which tests would fail if aux_data would be set as mandatory, those would be test_edr.py and test_xvg.py delivering the following error message once in test_edr.py and twice in test_xvg.py

with the error mesage being:

______________ TestEDRReader.test_raise_error_no_auxdata_provided ______________
self = <MDAnalysisTests.auxiliary.test_edr.TestEDRReader object at 0x779471f340b0>
ref = <MDAnalysisTests.auxiliary.test_edr.EDRReference object at 0x77947516a990>
ref_universe = <Universe with 33876 atoms>

`def test_raise_error_no_auxdata_provided(self, ref, ref_universe):
    with pytest.raises(ValueError, match="No input `auxdata`"):
        ref_universe.trajectory.add_auxiliary()
         TypeError: ProtoReader.add_auxiliary() missing 1 required positional argument: 'auxdata'`

all three of the errors would rely on the missing of the required auxdata argument

@talagayev
Copy link
Member Author

Quick follow-up on the reason of the errors during test_edr.py and twice in test_xvg.py if aux_data is set as mandatory.

The error in them stands from testsuite/MDAnalysisTests/auxiliary/base.py that has a specific test:

def test_raise_error_no_auxdata_provided(self, ref, ref_universe):

which expects an ValueError, which would occur in the variant where the aux_data is set as optional in add_auxiliary and has a condition:
if auxdata is None: raise ValueError

But gets instead an TypeError currently, now that it is missing aux_data as an mandatory argument. Thus the tests can be adjusted to check for an TypeError instead, which removes the errors in the tests, but then i am not sure if the condition:
if auxdata is None: raise ValueError should also be removed from package/MDAnalysis/coordinates/base.py

@IAlibay
Copy link
Member

IAlibay commented Mar 26, 2024

@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)

@hmacdope
Copy link
Member

@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?

@IAlibay
Copy link
Member

IAlibay commented Mar 26, 2024

:/ I think I understand your plan @hmacdope

I'll say this in two parts:

  • If this is GSoC related (sorry I am not keeping track of contributors), then there's plenty of precedent where we've approved but not merged a PR (for various reasons), so I would advise not seeing any decisions here as affecting contributor efforts. i.e. "symbolic merge" is a legit strategy
  • (let's move this discussion to an issue or elsewhere) I think it might be ok to "hold" PRs but we might need to come up with a policy for it. We're at least one (if not two) minor versions away, so roughly 6+ months given our current release schedule. It might be hard to do right by contributors if we don't have a good merge/release plan that everyone sticks to.

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.

Copy link
Member

@orbeckst orbeckst left a 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)
Copy link
Member

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)
Copy link
Member

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.

Copy link
Member Author

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)
Copy link
Member

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

@IAlibay IAlibay mentioned this pull request Mar 29, 2024
1 task
@talagayev
Copy link
Member Author

Should it stay right now as it is, with aux_data optional and be changed to mandatory later on in a follow-up PR?

@orbeckst
Copy link
Member

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 aux_data mandatory and aux_spec optional.

@orbeckst
Copy link
Member

I am not quite sure how we will implement deprecation/change warnings. That's a separate issue/PR, though.

@orbeckst
Copy link
Member

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.

@talagayev
Copy link
Member Author

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 aux_data mandatory and aux_spec optional.

Sounds good, will adjust it to make aux_data mandatory and leave aux_spec optional

@talagayev
Copy link
Member Author

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.

Done

@orbeckst
Copy link
Member

@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!

@hmacdope
Copy link
Member

hmacdope commented Apr 1, 2024

More than happy for this to count for GSOC.

@BFedder
Copy link
Contributor

BFedder commented Apr 1, 2024

:/ I think I understand your plan @hmacdope

I'll say this in two parts:

* If this is GSoC related (sorry I am not keeping track of contributors), then there's plenty of precedent where we've approved but not merged a PR (for various reasons), so I would advise not seeing any decisions here as affecting contributor efforts. i.e. "symbolic merge" is a legit strategy

* (let's move this discussion to an issue or elsewhere) I think it might be ok to "hold" PRs but we might need to come up with a policy for it. We're at least one (if not two) minor versions away, so roughly 6+ months given our current release schedule. It might be hard to do right by contributors if we don't have a good merge/release plan that everyone sticks to.

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.

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.

@RMeli RMeli added this to the Release 3.0 milestone Jun 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change order of arguments for adding auxiliary data for 3.0 release
8 participants