-
Notifications
You must be signed in to change notification settings - Fork 663
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
Filtering pytng reader warnings (resolves #3986) #4010
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 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.
Codecov ReportBase: 93.52% // Head: 93.52% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## develop #4010 +/- ##
========================================
Coverage 93.52% 93.52%
========================================
Files 190 190
Lines 25028 25028
Branches 3542 3542
========================================
Hits 23407 23407
Misses 1100 1100
Partials 521 521 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
@IAlibay I hope this is what you meant. The filter has been applied specifically for test_tng.py |
@ooprathamm could you please edit your original PR comment at the top to include the issue number that you're fixing as |
@ooprathamm just double checking do you have pytng installed? Otherwise the tests will be skipped. |
I verified on my own setup that warnings down from ~900 to ~80 so seems to work |
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.
LGTM!
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.
After some discussion @IAlibay and myself have decided we want to eliminate all the warnings.
Try adding the same filter to more test fixture classes until the warning count gets to 0 for the "Stride of block" warnings.
@hmacdope the difference in warnings seems to be from other optional dependencies in your system (for mine and as stated in #3986 ~600 to ~7 warnings) is there list of optional dependencies that I need to install other than pytng |
No just pytng should be fine! Try and get the last 7 gone and then we should be good to go. |
@hmacdope filtered all the warnings in test_tng also removed deprecation warning thrown for xdrlib in topology package |
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.
Just the one thing, also please add yourself to AUTHORS and your name to the top of the changelog (a changelog entry is not required). We also would ask if you could introduce yourself on the mailing list if you haven't done so (please include your github handle).
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.
Just the one thing from me.
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.
Looks good to me, I think you have already introduced yourself on the Mailing list, so good to go from me.
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.
lgtm, thanks!
Fixes #3986
Changes made in this Pull Request:
PR Checklist