-
Notifications
You must be signed in to change notification settings - Fork 652
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
Fix DeprecationWarning by using np.arctan2 for element-wise array ope… #4730
base: develop
Are you sure you want to change the base?
Fix DeprecationWarning by using np.arctan2 for element-wise array ope… #4730
Conversation
…rations in phase_ang calculation (NumPy 1.25+ compatibility)
Hello @laksh-krishna-sharma! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2024-10-15 12:19:10 UTC |
Linter Bot Results:Hi @laksh-krishna-sharma! 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 |
@orbeckst Sir please review the Pull Request |
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.
There are many changes in this PR, most of which have nothing to do with the original issue. Please only change what needs changing to address the issue. Otherwise it takes too much time to review. The fewer changes there are in the diff view the easier it is for reviewers.
If you think that this a file needs to be refactored, open an issue, please. Note that nuclinfo is slowly being transitioned to a more modern framework #3720 so we really only want minimal changes here.
Please also check the tests (click on any of the failing tests and read the output to see where they are failing): They fail from code that you changed so you can immediately see that your changes are not correct.
|
||
.. versionadded:: 0.7.6 |
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.
Do not remove docs, especially not versionadded.
ok @orbeckst thanks for your response , i will make sure to do minimal changes from now and to fix the issue as soon as posible. |
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.
Can you please create a PR that only fixes the issue and does not change any other code?
- make sure that the tests still pass
- demonstrate that before your changes you see the warning (e.g., just run the tests for nuclinfo:
pytest -v MDAnalysisTests/analysis/test_nuclinfo.py
) - demonstrate that after your changes you don't see the warning anymore.
You can copy and paste the output that shows warnings.
Look at the test output (click on Details in failing tests ❌ ) and you see that tests related to nuclinfo are now failing:
This means that your code changes introduced errors. You need to fix your code. |
Fixs #4339 : DeprecationWarning in
nuclinfo.py
for NumPy 1.25+ CompatibilityThis commit resolves a DeprecationWarning related to NumPy's handling of arrays with
ndim > 0
, which will raise an error in future releases. Specifically, the calculation ofphase_ang
has been updated to usenp.arctan2
, ensuring element-wise array operations are handled correctly.Changes made in this Pull Request:
atan2(D, C)
withnp.arctan2(D, C)
to avoid scalar conversion issues.This update ensures the codebase is future-proof for upcoming versions of NumPy.
PR Checklist
Developers certificate of origin
📚 Documentation preview 📚: https://mdanalysis--4730.org.readthedocs.build/en/4730/