Fix for issue #4655 Removing mutable data structures and function calls as default arguments in the entire codebase #4810

tanishy7777 · 2024-11-30T11:34:30Z

Fixes #4655

Changes made in this Pull Request:

Removing mutable data structures and function calls as default arguments in the entire codebase and initializing/calling them inside the function instead
There were 12 issues found using the ruff module(select = B006, B008 and ignore=E501) which were all fixed

PR Checklist

Tests
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--4810.org.readthedocs.build/en/4810/

…s as default arguments in the entire codebase

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 GitHub Discussions 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.

tanishy7777 · 2024-11-30T11:37:25Z

Before and after the changes were made (results below are from pytest tests in the testsuite)
Before

After the changes

codecov · 2024-11-30T11:43:57Z

Codecov Report

Attention: Patch coverage is 64.28571% with 10 lines in your changes missing coverage. Please review.

Project coverage is 93.62%. Comparing base (a27591c) to head (1c1c644).

Files with missing lines	Patch %	Lines
package/MDAnalysis/analysis/encore/similarity.py	62.50%	0 Missing and 3 partials ⚠️
package/MDAnalysis/analysis/base.py	0.00%	1 Missing and 1 partial ⚠️
package/MDAnalysis/analysis/helix_analysis.py	66.66%	1 Missing and 1 partial ⚠️
package/MDAnalysis/topology/ITPParser.py	33.33%	1 Missing and 1 partial ⚠️
package/MDAnalysis/analysis/align.py	66.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4810      +/-   ##
===========================================
- Coverage    93.68%   93.62%   -0.07%     
===========================================
  Files          177      189      +12     
  Lines        21743    22833    +1090     
  Branches      3055     3067      +12     
===========================================
+ Hits         20370    21377    +1007     
- Misses         927     1003      +76     
- Partials       446      453       +7

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

RMeli

Can you please detail how the error raised in #4810 (comment) is related to the changes?

package/CHANGELOG

RMeli · 2024-11-30T21:52:52Z

package/MDAnalysis/analysis/helix_analysis.py

@@ -387,11 +388,13 @@ class HELANAL(AnalysisBase):
        'local_screw_angles': (-2,),
    }

-    def __init__(self, universe, select='name CA', ref_axis=[0, 0, 1],
+    def __init__(self, universe, select='name CA', ref_axis=None,


What about using a tuple instead of a list?

Sure I can do that

I meant using it as an argument (instead of adding the logic with None). Because tuples are immutable (while lists are mutable).

I meant using it as an argument (instead of adding the logic with None). Because tuples are immutable (while lists are mutable).

Oh that makes sense

tanishy7777 · 2024-12-01T08:13:55Z

Can you please detail how the error raised in #4810 (comment) is related to the changes?

I am not sure why the error got resolved, but after making the changes it got solved.

According to the file test_encore from which the error was generated from, the function can result in failure sometimes because the function involves generating random numbers.

shortened the fixes in the changelog

using tuples instead of list for ref_axis

tanishy7777 · 2024-12-01T08:40:05Z

I have added the changes that you have mentioned @RMeli

package/MDAnalysis/analysis/encore/clustering/cluster.py

tylerjereddy

I'm "ok" with the diff modulo one or two outstanding comments from Rocco.

Any chance folks would be on board to "enforce" this in CI with ruff check package --select B006,B008? ruff is pretty fast and widely adopted now, and that would only check for these two rules and nothing else. Eventually using a ruff.toml might make sense, but maybe one step at a time...

package/MDAnalysis/analysis/encore/clustering/cluster.py

tylerjereddy · 2024-12-07T02:09:58Z

The CI fails are all unrelated--the usual rdkit failure observed in other recent PRs.

RMeli · 2024-12-07T20:07:19Z

Any chance folks would be on board to "enforce" this in CI with ruff check package --select B006,B008? ruff is pretty fast and widely adopted now, and that would only check for these two rules and nothing else. Eventually using a ruff.toml might make sense, but maybe one step at a time...

+1 for adding ruff with these rules. We went with black for formatting, but I think it would be good to have ruff for linting since it has some momentum. I can add it to the GitHub action limited to the few rules you suggest, then we can expand it with time and when formatting is over (addressing the listing issues not concerning formatting was the next step anyways).

RMeli · 2024-12-12T13:48:43Z

@tanishy7777 Why did you close this PR? Besides #4810 (comment) I think it was in a pretty good shape.

tanishy7777 · 2024-12-12T14:20:57Z

@RMeli I am really sorry I was trying to creating a new fork, and deleted the old one. I think that may have closed the PR

RMeli · 2024-12-13T10:07:18Z

No worries @tanishy7777, that happens. Are you going to re-open the PR from the new fork? I think it was almost ready to be merged, so it would be good to have. But if you don't want to work on it anymore, no worries at all.

tanishy7777 · 2024-12-13T12:52:19Z

No worries @tanishy7777, that happens. Are you going to re-open the PR from the new fork? I think it was almost ready to be merged, so it would be good to have. But if you don't want to work on it anymore, no worries at all.

Thanks for understanding. I will open the new PR from the new fork. I plan to just make all the changes locally on my pc again and then push that in the PR. I don't think there is a better way right? Since I deleted the old repo? @RMeli

tanishy7777 · 2024-12-13T12:52:41Z

Also just to clarify the reason u merged develop onto my commit is because I think I might have created a new branch using

git checkout -b <new-branch-name>

instead of

git checkout -b <new-branch-name> develop

Which also explains why the codecov percentage was so different? Is that right?

RMeli · 2024-12-13T14:12:46Z

@tanishy7777 I merged develop in this PR because #4640 fixed some issues with the tests, and therefore was needed to fix the tests running for this PR. With git pull you would have got the changes on your local repository as well. Sorry for the confusion.

I don't think there is a better way right?

Unfortunately, I don't think so. If you can't be bothered no worries, I can fix these issues myself if we decide to go ahead with #4825. Up to you if you want to spend more time on this.

tanishy7777 · 2024-12-13T14:21:29Z

@tanishy7777 I merged develop in this PR because #4640 fixed some issues with the tests, and therefore was needed to fix the tests running for this PR. With git pull you would have got the changes on your local repository as well. Sorry for the confusion.

Oh ok got it!

Unfortunately, I don't think so. If you can't be bothered no worries, I can fix these issues myself if we decide to go ahead with #4825. Up to you if you want to spend more time on this.

No problems! I have made the new PR in #4834
@RMeli

tanishy7777 added 3 commits November 30, 2024 16:21

Fix for issue 4655 removing mutable data structures and function call…

b3c5219

…s as default arguments in the entire codebase

formatting as per PEP8 and updated CHANGELOG

b1eb89f

removing unused imports which were added accidentally

c367273

github-actions bot reviewed Nov 30, 2024

View reviewed changes

tanishy7777 mentioned this pull request Nov 30, 2024

MAINT: a few mutable default arguments are back #4655

Closed

RMeli reviewed Nov 30, 2024

View reviewed changes

tanishy7777 added 2 commits December 1, 2024 13:46

Update CHANGELOG

48bfd5d

shortened the fixes in the changelog

Update helix_analysis.py

dc712ac

using tuples instead of list for ref_axis

Merge branch 'develop' into issue_4655

03197ad

RMeli reviewed Dec 6, 2024

View reviewed changes

package/MDAnalysis/analysis/encore/clustering/cluster.py Show resolved Hide resolved

tylerjereddy added the maintainability label Dec 7, 2024

tylerjereddy reviewed Dec 7, 2024

View reviewed changes

package/MDAnalysis/analysis/encore/clustering/cluster.py Show resolved Hide resolved

This was referenced Dec 7, 2024

Code formatting, style guide enforcement and static checks #2450

Open

Ruff #4825

Closed

add active PRs to black ignore #4815

Merged

Merge branch 'develop' into issue_4655

1c1c644

tanishy7777 closed this by deleting the head repository Dec 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for issue #4655 Removing mutable data structures and function calls as default arguments in the entire codebase #4810

Fix for issue #4655 Removing mutable data structures and function calls as default arguments in the entire codebase #4810

tanishy7777 commented Nov 30, 2024 •

edited by github-actions bot

Loading

github-actions bot left a comment

tanishy7777 commented Nov 30, 2024

codecov bot commented Nov 30, 2024 •

edited

Loading

RMeli left a comment

RMeli Nov 30, 2024

tanishy7777 Dec 1, 2024

RMeli Dec 6, 2024

tanishy7777 Dec 13, 2024

tanishy7777 commented Dec 1, 2024 •

edited

Loading

tanishy7777 commented Dec 1, 2024

tylerjereddy left a comment

tylerjereddy commented Dec 7, 2024

RMeli commented Dec 7, 2024

RMeli commented Dec 12, 2024

tanishy7777 commented Dec 12, 2024 •

edited

Loading

RMeli commented Dec 13, 2024

tanishy7777 commented Dec 13, 2024

tanishy7777 commented Dec 13, 2024 •

edited

Loading

RMeli commented Dec 13, 2024

tanishy7777 commented Dec 13, 2024 •

edited

Loading

Fix for issue #4655 Removing mutable data structures and function calls as default arguments in the entire codebase #4810

Fix for issue #4655 Removing mutable data structures and function calls as default arguments in the entire codebase #4810

Conversation

tanishy7777 commented Nov 30, 2024 • edited by github-actions bot Loading

PR Checklist

Developers certificate of origin

github-actions bot left a comment

Choose a reason for hiding this comment

tanishy7777 commented Nov 30, 2024

codecov bot commented Nov 30, 2024 • edited Loading

Codecov Report

RMeli left a comment

Choose a reason for hiding this comment

RMeli Nov 30, 2024

Choose a reason for hiding this comment

tanishy7777 Dec 1, 2024

Choose a reason for hiding this comment

RMeli Dec 6, 2024

Choose a reason for hiding this comment

tanishy7777 Dec 13, 2024

Choose a reason for hiding this comment

tanishy7777 commented Dec 1, 2024 • edited Loading

tanishy7777 commented Dec 1, 2024

tylerjereddy left a comment

Choose a reason for hiding this comment

tylerjereddy commented Dec 7, 2024

RMeli commented Dec 7, 2024

RMeli commented Dec 12, 2024

tanishy7777 commented Dec 12, 2024 • edited Loading

RMeli commented Dec 13, 2024

tanishy7777 commented Dec 13, 2024

tanishy7777 commented Dec 13, 2024 • edited Loading

RMeli commented Dec 13, 2024

tanishy7777 commented Dec 13, 2024 • edited Loading

tanishy7777 commented Nov 30, 2024 •

edited by github-actions bot

Loading

codecov bot commented Nov 30, 2024 •

edited

Loading

tanishy7777 commented Dec 1, 2024 •

edited

Loading

tanishy7777 commented Dec 12, 2024 •

edited

Loading

tanishy7777 commented Dec 13, 2024 •

edited

Loading

tanishy7777 commented Dec 13, 2024 •

edited

Loading