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

Enable Pylint warning unused-import #4518

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

Conversation

vachram97
Copy link

@vachram97 vachram97 commented Mar 18, 2024

Contributes to #1295 (unused-import)

Changes made in this Pull Request:

  • unused-import check was enabled for pylint
  • unnecessary imports were removed
  • 2 imports were shadowed since they are internal for library part

PR Checklist

  • Tests?
    some tests are skipped on local machine
  • Docs?
    no changes are needed to docs
  • CHANGELOG updated?
    as no new functionality/bug fixes were introduced, there is nothing to add to changelog
  • Issue raised/referenced?

Developers certificate of origin


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

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

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.

Copy link

github-actions bot commented Mar 18, 2024

Linter Bot Results:

Hi @vachram97! 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/9726590606/job/26845212202


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 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.15%. Comparing base (cfa4438) to head (e28a2a9).

Current head e28a2a9 differs from pull request most recent head a76213f

Please upload reports for the commit a76213f to get more accurate results.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #4518      +/-   ##
===========================================
- Coverage    93.60%   93.15%   -0.45%     
===========================================
  Files          171       12     -159     
  Lines        21235     1067   -20168     
  Branches      3933        0    -3933     
===========================================
- Hits         19876      994   -18882     
+ Misses         899       73     -826     
+ Partials       460        0     -460     

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

Copy link
Member

@RMeli RMeli left a comment

Choose a reason for hiding this comment

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

Hi @vachram97, thanks for your contribution. The linters failure appears to be un-related to this PR.

LGTM, but given the size of the changes, we will need a few more eyes on this.

Note to self: once this is squashed-merged, the commit will need to be added to .git-blame-ignore-revs.

Copy link
Member

@RMeli RMeli left a comment

Choose a reason for hiding this comment

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

Please add yourself in the AUTHORS file. And I would still add an entry in the CHANGELOG.

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.

Very quickly review, apologies please take the brief comments as me only having a few minutes to review and not a reflection on the quality of the work (thank you so much for taking this task!) - @RMeli feel free to discard my review once the big things have been addressed (parmed mostly)

@@ -117,7 +117,6 @@
.. autoexception:: ApplicationError

"""
Copy link
Member

Choose a reason for hiding this comment

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

We have no tests for this module - I would suggest not touching imports here

@@ -61,8 +61,7 @@
Path,
PSAPair,
PSAnalysis,
)

)
Copy link
Member

Choose a reason for hiding this comment

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

here and everywhere else this is done, please don't remove this extra blank line - it's there for pep8 reasons

WaterOrientationalRelaxation,
AngularDistribution,
MeanSquareDisplacement,
SurvivalProbability,
)

)
Copy link
Member

Choose a reason for hiding this comment

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

as above

package/MDAnalysis/coordinates/memory.py Show resolved Hide resolved
package/MDAnalysis/topology/ParmEdParser.py Show resolved Hide resolved
package/MDAnalysis/topology/core.py Show resolved Hide resolved
@orbeckst
Copy link
Member

@RMeli can you please shepherd this PR? Thank you!

@orbeckst orbeckst mentioned this pull request Mar 26, 2024
38 tasks
@orbeckst
Copy link
Member

@vachram97 I edited your issue text so that we don't accidentally close the parent issue #1295 .

@orbeckst
Copy link
Member

@vachram97 do you still have time to work on this PR? If so, please respond to the reviews.

@orbeckst orbeckst added the close? Evaluate if issue/PR is stale and can be closed. label May 31, 2024
@orbeckst
Copy link
Member

I merged and accidentally removed a new import in transformations.py. I added it back in 4cbe32c and hopefully that's now ok.

@vachram97
Copy link
Author

I have updated the PR with all comments were addressed as requested and last version MDAnalysis/develop was merged into PR.

Tests fail on the testsuite/MDAnalysisTests/parallelism/test_multiprocessing.py test with time limit error and seems to be not related to this PR.

@orbeckst orbeckst removed the close? Evaluate if issue/PR is stale and can be closed. label Jun 7, 2024
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 is a nice clean-up PR. There's one issue that needs fixing and some minor comments – please see comments. Please ping me when you I can do a final review.

@@ -117,7 +117,6 @@
.. autoexception:: ApplicationError

"""
import glob
Copy link
Member

Choose a reason for hiding this comment

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

Removing glob here is ok because it's imported when needed in the code.

Comment on lines 216 to +217
from MDAnalysis.lib.util import store_init_arguments
from MDAnalysis.lib.mdamath import triclinic_box
Copy link
Member

Choose a reason for hiding this comment

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

Can we change these to relative imports like everything else?

Suggested change
from MDAnalysis.lib.util import store_init_arguments
from MDAnalysis.lib.mdamath import triclinic_box
from ..lib.util import store_init_arguments
from ..lib.mdamath import triclinic_box

Comment on lines -24 to -25
from ..converters.ParmEd import (ParmEdConverter, ParmEdReader,
get_indices_from_subset, MDA2PMD,)
Copy link
Member

Choose a reason for hiding this comment

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

These MUST stay for compatibility reasons. The module will be removed in 3.0.0.

Just mark them to be ignored by the linter.

@@ -38,8 +38,6 @@

"""

from ..lib import util
from ..lib.mdamath import triclinic_box, triclinic_vectors, box_volume
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 ok because we did NOT advertise these functions to be available in coordinates.core (https://docs.mdanalysis.org/stable/documentation_pages/coordinates/core.html#module-MDAnalysis.coordinates.core) but be careful when removing imports in top level __init__.py or in anything named core.py or base.py modules.

@@ -31,7 +31,7 @@
import numpy as np

from . import base
from . import core
from ..lib.mdamath import triclinic_box
Copy link
Member

Choose a reason for hiding this comment

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

Do we like using triclinic_box() better than mdamath.triclinic_box()?

Personally, I generally prefer non-local objects to have at least one level of provenance attached to their name but I won't block over choosing the bare import.

Copy link
Member

Choose a reason for hiding this comment

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

Unless there are other reviewers that want to change to

from ..lib import mdamath

...
mdamath.triclinic_box(...)

leave it as is, at least it's consistently used throughout.

Comment on lines -170 to -172
from numpy.linalg import norm

from .mdamath import angle as vecangle
Copy link
Member

Choose a reason for hiding this comment

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

Removal is ok because it wasn't documented to be present here.

Copy link
Member

Choose a reason for hiding this comment

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

... and if it doesn't break our tests then it should be fine.

@@ -29,8 +29,7 @@

import MDAnalysis.analysis.distances

from numpy.testing import (assert_equal, assert_array_equal, assert_almost_equal,
assert_array_almost_equal,assert_allclose)
from numpy.testing import (assert_equal ,assert_allclose)
Copy link
Member

Choose a reason for hiding this comment

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

pep8 space and I don't think parentheses are needed

Suggested change
from numpy.testing import (assert_equal ,assert_allclose)
from numpy.testing import assert_equal, assert_allclose

@@ -24,7 +24,7 @@
from unittest.mock import patch

import MDAnalysis as mda
import MDAnalysis.analysis.gnm
from MDAnalysis.analysis.gnm import (GNMAnalysis, closeContactGNMAnalysis)
Copy link
Member

Choose a reason for hiding this comment

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

not sure that this is strictly necessary — I am not a fan of bare imports

@RMeli
Copy link
Member

RMeli commented Jun 29, 2024

@vachram97 I took the freedom to fix the conflicts for you, so that we can move forward and see how CI is faring.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants