Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Emit warnings for empty Ag and user grid in DensityAnalysis #3091
Emit warnings for empty Ag and user grid in DensityAnalysis #3091
Changes from 14 commits
fb491fd
7fec795
c965b77
d2d54fc
2b1c50d
c00e3f3
65e0c45
14acdfb
d0f5263
4257b45
e4fc9aa
8232192
52fd1f3
eff2854
ec9c1f3
f4319ee
0423a48
c7caaef
b861b34
d6d23d6
33c94fa
dab9ad9
7c22dc1
dc834c8
4e8008e
a83c27b
6675097
d22916c
f3af280
245a859
9c127f5
e652669
e3990ee
5a2b371
10db351
9341947
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Couple of points to consider here;
Is this the right place to be putting this? Given the context of the original issue, and the need to make it so that the user defined grid still works even if there are no atoms in the frame, would placing this elsewhere be more appropriate?
Doing a
return
post-warning is a non-safe way to handle things. It's worth considering what the difference between a "warning" and an "error" is. If you're issuing a warning, you are telling users that non-expected behaviour is likely to occur (and why) but that the code will carry on. If you raise an error, you are telling users that something wrong/unexpected has occurred and you are aborting further code execution. In this case, if you are attempting to abort execution (as you are viareturn
), then an error (of appropriate type) should be raised instead.I think the way this is written probably needs to be changed, but it's worth mentioning anyways. Doing a return equality check on method return that essentially returns a
Boolean
is a non-pythonic way to handle things. If you're checking forTrue
/False
, the best way to do things is to have the method you're calling returnTrue
/False
and then just do: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.
Reading your replies to some of the other comments, I think it might be worth you investing a bit more time in understanding how the
AnalysisBase
class works, particularly the order in which each of these class methods get called.Have a look at
MDAnalysis.analysis.base
with the aim to conceptualise what_single_frame
and_conclude
mean in terms of analyzing a Molecular Dynamics trajectory. Once you've done this, it's then worth going back to the original issue and trying to understand where the edge case is causing the code to fail and why.Unless I'm missing something (which is always very much a possibility), doing these checks here may not be appropriate?
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 mentioned above in
_single_frame
.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.
I wonder if three duplications of the same code block is enough to justify abstraction to a single i.e., function. Maybe borderline if the
return
has to remain outside of the abstraction I suppose.On a more basic level, just asking--the code will still produce the original failure/exception (is that what it was doing?) and this will simply add a warning to clarify?
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 might sound very primitive but my logic was to return out of the function and warn the user of no atoms until atomgroup gets updated with atleast one atom. In the Issue, the user wanted to avoid the program crashing and would provide a dummy atom to keep the density frames generating. I tried to fix this by preventing a crash at the initializing, frame generation and conclusion. Perhaps a more efficient way is to make a function to check for no atoms and return.
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.
Again, my understanding of the DensityAnalysis function process may not be very accurate so I apologize for any incorrect arguments I make :P
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.
I'll be honest, this flake8 issue makes no sense to me, visual indentation should start at
len
not at its argument.Either way it's a minor thing so I'll let the other reviewers make a decision if they want here, but I'm willing to let it go.
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.
I ran this file through autopep8.
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 bizzare to me. I would revert this.
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.
@IAlibay , Do you want me to revert these changes? I feel like these changes might cause issues..
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.
I'm not really following what happened here. Why do you think these changes are causing issues?
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 a few changes which were made by autoPEP8 and not a part of fixing the error. The only failing check of CodeCov is because these changes are not covered by tests..
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.
It's up to you, codecov is complaining because it's gone from one line of untested code to two. The best solution here would be to go ahead and add a test for this, but otherwise I think we can live with just a style change.
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.
Sounds good! Thanks @IAlibay :)
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.
If there are no tests for these altered lines of code then we should have them.
I am tending towards: If you touch code it's your responsibility to test it.
Even if it looks as if it's just cosmetic, it might not be. Bugs can be subtle. Either leave it or test it.
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.
Breaking lines with
\
ends up introducing a PEP8 issue, it also ends up making the code harder to read. It's always worth noting that PEP8 is a guideline rather than a specific set of rules, code readability is always the end goal.That being said, there is an easy way to make this readable and also less than 79 characters in one line by using
f-strings
: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.
Thanks for this! Noted