-
Notifications
You must be signed in to change notification settings - Fork 646
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
base: develop
Are you sure you want to change the base?
Conversation
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.
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.
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.
Please have a look at the Please note: The |
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
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.
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
.
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.
Please add yourself in the AUTHORS
file. And I would still add an entry in the CHANGELOG
.
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.
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 | |||
|
|||
""" |
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.
We have no tests for this module - I would suggest not touching imports here
package/MDAnalysis/analysis/psa.py
Outdated
@@ -61,8 +61,7 @@ | |||
Path, | |||
PSAPair, | |||
PSAnalysis, | |||
) | |||
|
|||
) |
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.
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, | ||
) | ||
|
||
) |
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.
as above
@RMeli can you please shepherd this PR? Thank you! |
@vachram97 I edited your issue text so that we don't accidentally close the parent issue #1295 . |
@vachram97 do you still have time to work on this PR? If so, please respond to the reviews. |
add back no_copy_shim import in transformations
I merged and accidentally removed a new import in transformations.py. I added it back in 4cbe32c and hopefully that's now ok. |
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 |
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.
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 |
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.
Removing glob
here is ok because it's imported when needed in the code.
from MDAnalysis.lib.util import store_init_arguments | ||
from MDAnalysis.lib.mdamath import triclinic_box |
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 we change these to relative imports like everything else?
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 |
from ..converters.ParmEd import (ParmEdConverter, ParmEdReader, | ||
get_indices_from_subset, MDA2PMD,) |
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.
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 |
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.
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 |
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 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.
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.
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.
from numpy.linalg import norm | ||
|
||
from .mdamath import angle as vecangle |
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.
Removal is ok because it wasn't documented to be present here.
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.
... 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) |
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.
pep8 space and I don't think parentheses are needed
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) |
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.
not sure that this is strictly necessary — I am not a fan of bare imports
@vachram97 I took the freedom to fix the conflicts for you, so that we can move forward and see how CI is faring. |
Contributes to #1295 (unused-import)
Changes made in this Pull Request:
PR Checklist
some tests are skipped on local machine
no changes are needed to docs
as no new functionality/bug fixes were introduced, there is nothing to add to changelog
Developers certificate of origin
📚 Documentation preview 📚: https://mdanalysis--4518.org.readthedocs.build/en/4518/