Updated cimport for numpy to maintain cimport consistency #4346

Sumit112192 · 2023-11-15T10:31:36Z

Fixes #3908

Changes made in this Pull Request:

Have updated all cimport numpy from np to cnp

PR Checklist

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

Developers certificate of origin

I certify that this contribution is covered by the LGPLv2.1+ license as defined in our LICENSE and adheres to the Developer Certificate of Origin.

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

github-actions

Hello there first time contributor! Welcome to the MDAnalysis community! We ask that all contributors abide by our Code of Conduct and that first time contributors introduce themselves on the developer mailing list so we can get to know you. You can learn more about participating here. Please also add yourself to package/AUTHORS as part of this PR.

github-actions · 2023-11-15T10:33:55Z

Linter Bot Results:

Hi @Sumit112192! Thanks for making this PR. We linted your code and found the following:

There are currently no issues detected! 🎉

codecov · 2023-11-15T10:41:42Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (37d8fee) 93.37% compared to head (491a041) 93.37%.

Additional details and impacted files

@@            Coverage Diff             @@
##           develop    #4346     +/-   ##
==========================================
  Coverage    93.37%   93.37%             
==========================================
  Files          170      184     +14     
  Lines        22340    23453   +1113     
  Branches      4085     4085             
==========================================
+ Hits         20859    21899   +1040     
- Misses         963     1036     +73     
  Partials       518      518

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

RMeli · 2023-11-16T18:36:53Z

Thanks for the PR @Sumit112192. Looks like a few tests timed out, I'll restart them before having a deeper look.

RMeli · 2023-11-16T22:10:21Z

In the meantime, please add yourself to the AUTHORS file.

IAlibay · 2023-11-16T22:13:05Z

I believe part of the work that needs doing is checking (maybe adding) benchmarks to ensure that performance isn't affected? If I remember correctly that was an ask from @hmacdope last time someone started a PR on this.

RMeli · 2023-11-16T22:44:45Z

Indeed: #3978 (comment)

hmacdope · 2023-11-16T22:59:29Z

@Sumit112192 @IAlibay @RMeli Yes, thanks for taking an intial look here. The issue is that there is a difference in speed calling the C vs normal numpy api, and there are some cases where the C API is less appropriate. Previously we used aliasing which was unclear, but we just need to run the benchmarks to make sure there are no major performance regressions. The way to do this is https://github.com/MDAnalysis/mdanalysis/blob/develop/benchmarks/README.rst. Something like asv continuous <commit on main> <commit at HEAD of this PR >

hmacdope · 2023-11-16T22:59:54Z

I imagine if anything performance will improve but we should check

Sumit112192 · 2023-11-17T05:18:58Z

@hmacdope, I am getting an issue with asv.conf.json. In that file, it mentions using python 3.8 version. But in env/09b6664d368df460776d8e73bd3171a0/project/package/pyproject.toml it mentions to use python 3.9+

I ran the command asv continuous 433603 f4005d -e --verbose and got some error regarding python versions and I believe the error was related to the above discrepancy in python versions.

The error

`(my-project-env) sam@sam-Latitude-5480:~/mdanalysis/benchmarks$ asv continuous 433603 f4005d -e --verbose
· Running '/usr/bin/git config init.defaultBranch'
· Running '/usr/bin/git config --global init.defaultBranch'
· Running '/usr/bin/git config --system init.defaultBranch'
· Running '/usr/bin/git rev-parse f4005d^{commit}'
  OUTPUT -------->
  f4005db383f3f4c446433fb32156236f29f251ba
· Running '/usr/bin/git rev-parse 433603^{commit}'
  OUTPUT -------->
  43360341b56823ccc2459a0cf9cc709c2b30283f
· Running '/usr/bin/git config init.defaultBranch'
· Running '/usr/bin/git config --global init.defaultBranch'
· Running '/usr/bin/git config --system init.defaultBranch'
· Creating environments
·· Running '/home/sam/mdanalysis/benchmarks/env/09b6664d368df460776d8e73bd3171a0/bin/python -c pass'
· Discovering benchmarks
·· Running '/usr/bin/git rev-parse develop^{commit}'
   OUTPUT -------->
   f4005db383f3f4c446433fb32156236f29f251ba
·· Running '/usr/bin/git -c protocol.file.allow=always submodule deinit -f .'
·· Running '/usr/bin/git checkout -f f4005db383f3f4c446433fb32156236f29f251ba'
   OUTPUT -------->
   Previous HEAD position was 43360341b Updated cimport for numpy to maintain cimport consistency
   HEAD is now at f4005db38 Expose C distance backend in libmdanalysis.pxd (#4342)
·· Running '/usr/bin/git clean -fdx'
·· Running '/usr/bin/git -c protocol.file.allow=always submodule update --init --recursive'
·· Uninstalling from conda-py3.8-Cython-MDAnalysisTests-mock-nose-numpy-pytest-scipy-six
·· Running '/home/sam/mdanalysis/benchmarks/env/09b6664d368df460776d8e73bd3171a0/bin/python -mpip uninstall -y mdanalysis'
   OUTPUT -------->
   WARNING: Skipping mdanalysis as it is not installed.
·· Running '/usr/bin/git name-rev --name-only --exclude=remotes/* --no-undefined f4005db383f3f4c446433fb32156236f29f251ba'
   OUTPUT -------->
   develop
·· Building f4005db3 <develop> for conda-py3.8-Cython-MDAnalysisTests-mock-nose-numpy-pytest-scipy-six
·· Running '/home/sam/mdanalysis/benchmarks/env/09b6664d368df460776d8e73bd3171a0/bin/python -mpip wheel --no-deps --no-index -w /home/sam/mdanalysis/benchmarks/env/09b6664d368df460776d8e73bd3171a0/asv-build-cache/f4005db383f3f4c446433fb32156236f29f251ba /home/sam/mdanalysis/benchmarks/env/09b6664d368df460776d8e73bd3171a0/project/package'
   OUTPUT -------->
   Processing /home/sam/mdanalysis/benchmarks/env/09b6664d368df460776d8e73bd3171a0/project/package
     Preparing metadata (pyproject.toml): started
     Preparing metadata (pyproject.toml): finished with status 'error'
     error: subprocess-exited-with-error
     
     × Preparing metadata (pyproject.toml) did not run successfully.
     │ exit code: 255
     ╰─> [2 lines of output]
         MDAnalysis requires Python 3.9+. Python 3.8 detected
         Please upgrade your version of Python.
         [end of output]
   
     note: This error originates from a subprocess, and is likely not a problem with pip.
   error: metadata-generation-failed

   
   × Encountered error while generating package metadata.
   ╰─> See above for output.
   
   note: This is an issue with the package mentioned above, not pip.
   hint: See above for details.

·· Error running /home/sam/mdanalysis/benchmarks/env/09b6664d368df460776d8e73bd3171a0/bin/python -mpip wheel --no-deps --no-index -w /home/sam/mdanalysis/benchmarks/env/09b6664d368df460776d8e73bd3171a0/asv-build-cache/f4005db383f3f4c446433fb32156236f29f251ba /home/sam/mdanalysis/benchmarks/env/09b6664d368df460776d8e73bd3171a0/project/package (exit status 1)
·· Failed: trying different commit/environment
·· Running '/usr/bin/git -c protocol.file.allow=always submodule deinit -f .'
·· Running '/usr/bin/git checkout -f 43360341b56823ccc2459a0cf9cc709c2b30283f'
   OUTPUT -------->
   Previous HEAD position was f4005db38 Expose C distance backend in libmdanalysis.pxd (#4342)
   HEAD is now at 43360341b Updated cimport for numpy to maintain cimport consistency
·· Running '/usr/bin/git clean -fdx'
·· Running '/usr/bin/git -c protocol.file.allow=always submodule update --init --recursive'
·· Uninstalling from conda-py3.8-Cython-MDAnalysisTests-mock-nose-numpy-pytest-scipy-six
·· Running '/home/sam/mdanalysis/benchmarks/env/09b6664d368df460776d8e73bd3171a0/bin/python -mpip uninstall -y mdanalysis'
   OUTPUT -------->
   WARNING: Skipping mdanalysis as it is not installed.
·· Running '/usr/bin/git name-rev --name-only --exclude=remotes/* --no-undefined 43360341b56823ccc2459a0cf9cc709c2b30283f'
   OUTPUT -------->
   shiny-new-fix
·· Building 43360341 <shiny-new-fix> for conda-py3.8-Cython-MDAnalysisTests-mock-nose-numpy-pytest-scipy-six
·· Running '/home/sam/mdanalysis/benchmarks/env/09b6664d368df460776d8e73bd3171a0/bin/python -mpip wheel --no-deps --no-index -w /home/sam/mdanalysis/benchmarks/env/09b6664d368df460776d8e73bd3171a0/asv-build-cache/43360341b56823ccc2459a0cf9cc709c2b30283f /home/sam/mdanalysis/benchmarks/env/09b6664d368df460776d8e73bd3171a0/project/package'
   OUTPUT -------->
   Processing /home/sam/mdanalysis/benchmarks/env/09b6664d368df460776d8e73bd3171a0/project/package
     Preparing metadata (pyproject.toml): started
     Preparing metadata (pyproject.toml): finished with status 'error'
     error: subprocess-exited-with-error
     
     × Preparing metadata (pyproject.toml) did not run successfully.
     │ exit code: 255
     ╰─> [2 lines of output]
         MDAnalysis requires Python 3.9+. Python 3.8 detected
         Please upgrade your version of Python.
         [end of output]
   
     note: This error originates from a subprocess, and is likely not a problem with pip.
   error: metadata-generation-failed

   
   × Encountered error while generating package metadata.
   ╰─> See above for output.
   
   note: This is an issue with the package mentioned above, not pip.
   hint: See above for details.

·· Error running /home/sam/mdanalysis/benchmarks/env/09b6664d368df460776d8e73bd3171a0/bin/python -mpip wheel --no-deps --no-index -w /home/sam/mdanalysis/benchmarks/env/09b6664d368df460776d8e73bd3171a0/asv-build-cache/43360341b56823ccc2459a0cf9cc709c2b30283f /home/sam/mdanalysis/benchmarks/env/09b6664d368df460776d8e73bd3171a0/project/package (exit status 1)
·· Failed to build the project and import the benchmark suite.`

Sumit112192 · 2023-11-17T15:10:59Z

I changed the Python version in asv.conf.json to 3.9 to run the benchmark test.
Attached are the benchmark results:
benchmarkOutput.log

Can you explain what the results are? @hmacdope @RMeli
They are in --verbose mode, so the output is too large. Should I run it without verbose, so it might be easier to go through?

hmacdope · 2023-11-19T02:11:31Z

There should be a little summary at the end that compares performance of the two runs. Try again without verbose?

Sumit112192 · 2023-11-19T16:28:04Z

@hmacdope, I tried without verbose. But it was still without any summary.

hmacdope · 2023-12-05T00:12:10Z

@Sumit112192 I will run benchmarks for you, sorry for delay.

tylerjereddy

The diff looks "ok" to me. The benchmark suite is currently broken because of gh-3519 when I check locally, and it looks like several other issues may be present as well.

tylerjereddy · 2023-12-10T01:36:37Z

FWIW, I'd probably just vote to merge it. The benchmark suite is a bit of a mess at the moment it seems. I'll see if I can re-run with the failing benchmarks skipped using a regex, but it is annoying to do.

hmacdope · 2023-12-10T01:38:18Z

Similar thing to report, no performance regressions observed on my run. Benchmarks not all running at @tylerjereddy observed, but I am also pretty happy with the diff. I will approve.

hmacdope

Just changelog.

package/CHANGELOG

hmacdope · 2023-12-10T01:40:01Z

Also @Sumit112192 is this your first contribution? If so add yourself to AUTHORS.

Sumit112192 · 2023-12-10T05:59:18Z

@hmacdope I force-pushed the latest commit in which I updated the CHANGELOG and AUTHORS files. Is there any issue with the force commit?

Sumit112192 · 2023-12-10T06:08:08Z

@hmacdope I read that force pushing can be dangerous as it deletes other commit history. How can I delete my above force push? Or should I create another pull request using normal push?

hmacdope · 2023-12-10T06:10:22Z

@hmacdope I read that force pushing can be dangerous as it deletes other commit history. How can I delete my above force push? Or should I create another pull request using normal push?

The diff looks fine to me. no easy way to fix a force push but it's OK, your PR will all be squashed into one commit. No need to stress. We can continue here on this PR

Sumit112192 · 2023-12-10T06:15:30Z

@hmacdope relieved to hear that it is not a big issue. Also, I would like to work on the benchmark issue thing. How to get started on it? Any literature, I can follow to learn more about it?

Sumit112192 · 2023-12-10T06:17:02Z

That closed thing happened by mistake. Sorry for that.

hmacdope

Just the newest first change, might have got lost in the force push, but if you could just make the change again should be good to go.

package/CHANGELOG

hmacdope

Great work @Sumit112192! Super happy that you managed to improve our cimport consistency. Has been needed for a while. :) I will wait for @tylerjereddy to also approve, then good to roll.

tylerjereddy · 2023-12-11T18:30:21Z

I squash-merged per MDA convention (I think), thanks!

Only two files had changed since my positive review, and they were just author/changelog things.

I doubt there will be any performance surprises, but hopefully our benchmarks are in better shape soon.

github-actions bot added Component-Analysis Component-lib Component-Readers labels Nov 15, 2023

github-actions bot reviewed Nov 15, 2023

View reviewed changes

hmacdope mentioned this pull request Dec 5, 2023

Change benchmarks default python to be >3.8 #4351

Closed

tylerjereddy reviewed Dec 10, 2023

View reviewed changes

hmacdope requested changes Dec 10, 2023

View reviewed changes

package/CHANGELOG Outdated Show resolved Hide resolved

Sumit112192 added 2 commits December 10, 2023 11:14

Updated cimport for numpy to maintain cimport consistency

a8c5078

Updated CHANGELOG and AUTHORS file

f95a74f

Sumit112192 closed this Dec 10, 2023

Sumit112192 reopened this Dec 10, 2023

hmacdope requested changes Dec 11, 2023

View reviewed changes

package/CHANGELOG Outdated Show resolved Hide resolved

Update CHANGELOG

491a041

hmacdope approved these changes Dec 11, 2023

View reviewed changes

tylerjereddy merged commit 6179b2b into MDAnalysis:develop Dec 11, 2023
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated cimport for numpy to maintain cimport consistency #4346

Updated cimport for numpy to maintain cimport consistency #4346

Sumit112192 commented Nov 15, 2023 •

edited

Loading

github-actions bot left a comment

github-actions bot commented Nov 15, 2023 •

edited

Loading

codecov bot commented Nov 15, 2023 •

edited

Loading

RMeli commented Nov 16, 2023

RMeli commented Nov 16, 2023

IAlibay commented Nov 16, 2023

RMeli commented Nov 16, 2023

hmacdope commented Nov 16, 2023

hmacdope commented Nov 16, 2023

Sumit112192 commented Nov 17, 2023 •

edited by RMeli

Loading

Sumit112192 commented Nov 17, 2023 •

edited

Loading

hmacdope commented Nov 19, 2023

Sumit112192 commented Nov 19, 2023

hmacdope commented Dec 5, 2023

tylerjereddy left a comment

tylerjereddy commented Dec 10, 2023

hmacdope commented Dec 10, 2023

hmacdope left a comment

hmacdope commented Dec 10, 2023

Sumit112192 commented Dec 10, 2023

Sumit112192 commented Dec 10, 2023

hmacdope commented Dec 10, 2023

Sumit112192 commented Dec 10, 2023 •

edited

Loading

Sumit112192 commented Dec 10, 2023 •

edited

Loading

hmacdope left a comment

hmacdope left a comment

tylerjereddy commented Dec 11, 2023

Updated cimport for numpy to maintain cimport consistency #4346

Updated cimport for numpy to maintain cimport consistency #4346

Conversation

Sumit112192 commented Nov 15, 2023 • edited Loading

PR Checklist

Developers certificate of origin

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 15, 2023 • edited Loading

Linter Bot Results:

codecov bot commented Nov 15, 2023 • edited Loading

Codecov Report

RMeli commented Nov 16, 2023

RMeli commented Nov 16, 2023

IAlibay commented Nov 16, 2023

RMeli commented Nov 16, 2023

hmacdope commented Nov 16, 2023

hmacdope commented Nov 16, 2023

Sumit112192 commented Nov 17, 2023 • edited by RMeli Loading

Sumit112192 commented Nov 17, 2023 • edited Loading

hmacdope commented Nov 19, 2023

Sumit112192 commented Nov 19, 2023

hmacdope commented Dec 5, 2023

tylerjereddy left a comment

Choose a reason for hiding this comment

tylerjereddy commented Dec 10, 2023

hmacdope commented Dec 10, 2023

hmacdope left a comment

Choose a reason for hiding this comment

hmacdope commented Dec 10, 2023

Sumit112192 commented Dec 10, 2023

Sumit112192 commented Dec 10, 2023

hmacdope commented Dec 10, 2023

Sumit112192 commented Dec 10, 2023 • edited Loading

Sumit112192 commented Dec 10, 2023 • edited Loading

hmacdope left a comment

Choose a reason for hiding this comment

hmacdope left a comment

Choose a reason for hiding this comment

tylerjereddy commented Dec 11, 2023

Sumit112192 commented Nov 15, 2023 •

edited

Loading

github-actions bot commented Nov 15, 2023 •

edited

Loading

codecov bot commented Nov 15, 2023 •

edited

Loading

Sumit112192 commented Nov 17, 2023 •

edited by RMeli

Loading

Sumit112192 commented Nov 17, 2023 •

edited

Loading

Sumit112192 commented Dec 10, 2023 •

edited

Loading

Sumit112192 commented Dec 10, 2023 •

edited

Loading