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

Allow distance routines to accept an NumPy Array or AtomGroup [Core] #3730

Merged
merged 63 commits into from
Jul 22, 2022

Conversation

hmacdope
Copy link
Member

@hmacdope hmacdope commented Jun 23, 2022

Fixes #3708
Successor to #3718

Second attempt at addressing #3708 using a python only design. CZI_EOSS4 performance track.

Changes made in this Pull Request:

  • Change @check_coords to take also AtomGroups and return positions
  • Atomgroup parsing in @check_coords must be enabled with argument allow_atomgroup with default False maintaining current behaviour
  • Added type hinting to lots of distances module
  • Change distance_array, self_distance_array, calc_bonds, calc_angles, calc_dihedrals, and apply_PBC to take an atomgroup
  • Added tests for AtomGroup input to distances. Still some code duplication here can be possibly cleaned up .

Do we want to also change nsgrid functions, and the transform_ functions? If so is that best done here or separate PR?

PR Checklist

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

@hmacdope hmacdope requested a review from IAlibay June 30, 2022 00:08
@hmacdope hmacdope changed the title Allow distance routines to accept an NumPy Array or AtomGroup [Core, WIP] Allow distance routines to accept an NumPy Array or AtomGroup [Core] Jun 30, 2022
Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

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

Not 100% sure why some of this isn't being picked up by pep8speaks, anyways just a couple of PEP8 changes for the lines that changed (I'm ignoring one of the pep8speaks one because 81 char >> readable than 79 char in that occasion).

testsuite/MDAnalysisTests/lib/test_distances.py Outdated Show resolved Hide resolved
testsuite/MDAnalysisTests/lib/test_distances.py Outdated Show resolved Hide resolved
testsuite/MDAnalysisTests/lib/test_distances.py Outdated Show resolved Hide resolved
package/MDAnalysis/lib/util.py Outdated Show resolved Hide resolved
package/MDAnalysis/lib/util.py Outdated Show resolved Hide resolved
@hmacdope
Copy link
Member Author

hmacdope commented Jul 5, 2022

Thanks @IAlibay much appreciated.

@hmacdope
Copy link
Member Author

hmacdope commented Jul 6, 2022

Hmmmm still getting what looks to me like NumPy versioning issues on the Azure pipelines despite #3737 merged?

@IAlibay
Copy link
Member

IAlibay commented Jul 6, 2022

How odd, was developed merged into this branch?

@hmacdope
Copy link
Member Author

hmacdope commented Jul 6, 2022

Yeah it looks all up to date AFAIK. Log shows #3737 as merged in my history.

@IAlibay
Copy link
Member

IAlibay commented Jul 6, 2022

@hmacdope - Here we go this is the problem: https://github.com/numpy/numpy/releases/tag/v1.21.0

"A runtime-subcriptable alias has been added for ndarray"

Essentially NDArray isn't going to work in 1.20... That's a bit of a pain, I guess our options here are to either vendor the alias somewhere in our code or break NEP29. We should probably move this to a new issue since it's a pretty big decision.

@orbeckst
Copy link
Member

orbeckst commented Jul 6, 2022

That's just related to typing, though, right?

It looks to me that typing will remain a moving target for some time so my view is to do the best you can within current constraints and then move on. Add a comment how to do it nicely when np 1.21 is lay of the land and then put in something ugly or less specific.

I recognize that there are different views and mine is just the one of an outsider as far as typing goes...

@IAlibay
Copy link
Member

IAlibay commented Jul 6, 2022

That's just related to typing, though, right?

It looks to me that typing will remain a moving target for some time so my view is to do the best you can within current constraints and then move on. Add a comment how to do it nicely when np 1.21 is lay of the land and then put in something ugly or less specific.

I recognize that there are different views and mine is just the one of an outsider as far as typing goes...

Hugo will open an issue later today, but one of the things we'll need to discuss is what we do with the outreachy typing project since I think they've already started using npt.NDArray

@orbeckst
Copy link
Member

orbeckst commented Jul 6, 2022

Hugo will open an issue later today, but one of the things we'll need to discuss is what we do with the outreachy typing project since I think they've already started using npt.NDArray

Ahhh... and that's not been caught in tests with lowest supported versions? Alright, that's discussion that should happen on its own issue.

@hmacdope
Copy link
Member Author

hmacdope commented Jul 7, 2022

Pending discussion on #3748

@hmacdope
Copy link
Member Author

Other than the flaky tests this should be good to go again following discussion in #3748.

@hmacdope
Copy link
Member Author

@MDAnalysis/coredevs any chance I could get a second review here? It would be good to get this and #3751 merged soonish. :)

@IAlibay
Copy link
Member

IAlibay commented Jul 17, 2022

@hmacdope can you address whatever can be addressed from pep8speaks (all seems reasonable stuff like two lines vs 1). And get CI to run green. I'll review tomorrow once that's done.

Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

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

@hmacdope here are all the suggested changes that were left.

I'll eagerly approve with the hope that you'll handle these suggestions before merging this PR.

package/MDAnalysis/lib/distances.py Show resolved Hide resolved
package/MDAnalysis/lib/distances.py Show resolved Hide resolved
package/MDAnalysis/lib/distances.py Show resolved Hide resolved
package/MDAnalysis/lib/distances.py Show resolved Hide resolved
package/MDAnalysis/lib/distances.py Show resolved Hide resolved
package/MDAnalysis/lib/distances.py Show resolved Hide resolved
package/MDAnalysis/lib/distances.py Show resolved Hide resolved
package/MDAnalysis/lib/distances.py Show resolved Hide resolved
package/MDAnalysis/lib/util.py Outdated Show resolved Hide resolved
testsuite/MDAnalysisTests/lib/test_distances.py Outdated Show resolved Hide resolved
@hmacdope
Copy link
Member Author

Thanks @IAlibay, sorry I swear I made the changes before I pinged but I think I confused myself with #3751.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component-lib CZI-performance performance track of CZIEOSS4 grant enhancement
Projects
Development

Successfully merging this pull request may close these issues.

Provide Ndarray or AtomGroup to distance routines
6 participants