-
Notifications
You must be signed in to change notification settings - Fork 667
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
Changes from 23 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
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -369,6 +369,7 @@ class DensityAnalysis(AnalysisBase): | |
.. versionchanged:: 2.0.0 | ||
:func:`_set_user_grid` is now a method of :class:`DensityAnalysis`. | ||
""" | ||
|
||
IAlibay marked this conversation as resolved.
Show resolved
Hide resolved
|
||
def __init__(self, atomgroup, delta=1.0, | ||
metadata=None, padding=2.0, | ||
gridcenter=None, | ||
|
@@ -394,8 +395,18 @@ def _prepare(self): | |
logger.warning(msg) | ||
# Generate a copy of smin/smax from coords to later check if the | ||
# defined box might be too small for the selection | ||
smin = np.min(coord, axis=0) | ||
smax = np.max(coord, axis=0) | ||
try: | ||
IAlibay marked this conversation as resolved.
Show resolved
Hide resolved
|
||
smin = np.min(coord, axis=0) | ||
smax = np.max(coord, axis=0) | ||
except ValueError as err: | ||
msg = ("No atoms in AtomGroup at input time frame. " | ||
"Grid for density could not be automatically" | ||
" generated. A user defined grid was found" | ||
" and is being used as the density grid") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My example message for the other message can't be directly copied here, the context is different. It's incorrect to say that you were attempting to "automatically generate" the grid since it was manually set by the user. Please amend this to better fit the context of the warning. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's worth considering why we are emitting the warning. Is it reasonable to be warning folks that we're following expected operation (i.e. they asked to use a user defined grid, is it informative to have us tell them we're doing this)? Rather than thinking about what the code is doing next, it's best to phrase the warning in terms of the consequences of not having any atoms in the atomgroup. What does this mean in terms of the user grid? How does it impact us checking if the user grid is large enough to accommodate the selected atoms? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, that would mean that the user should have made sure that they enter a grid large enough to accomodate atoms in the next frames. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure "large range" is the right set of words here. Maybe something like "This be may be intended; please ensure that your grid selection covers the atomic positions you wish to capture."? |
||
warnings.warn(msg) | ||
logger.warning(msg) | ||
smin = self._gridcenter | ||
smax = self._gridcenter | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are Please explain the logic here. What consequences does it have downstream? Does the code fail further downstream if the user has not supplied a proper grid selection? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, that was done because There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you do # need to be set for _set_user_grid() but will be
# overriden by user-defined values
smin = smax = None to make clearer what's happening? If There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It will trigger this warning in set_user_grid if I do that. The check is meant for regular situations to compare the atom selection with user-defined grid. In the case of no atoms in selection, I just want it to pass through. So setting smin and smax at the gridcenter will always pass the comparison with a user defined box. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Document in a comment, please.
orbeckst marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a comment explaining the hacky assignment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done! :) |
||
# Overwrite smin/smax with user defined values | ||
smin, smax = self._set_user_grid(self._gridcenter, self._xdim, | ||
hmacdope marked this conversation as resolved.
Show resolved
Hide resolved
|
||
self._ydim, self._zdim, smin, | ||
|
@@ -408,8 +419,16 @@ def _prepare(self): | |
# ideal solution would use images: implement 'looking across the | ||
# periodic boundaries' but that gets complicated when the box | ||
# rotates due to RMS fitting. | ||
smin = np.min(coord, axis=0) - self._padding | ||
smax = np.max(coord, axis=0) + self._padding | ||
try: | ||
smin = np.min(coord, axis=0) - self._padding | ||
smax = np.max(coord, axis=0) + self._padding | ||
except ValueError as err: | ||
errmsg = ("No atoms in AtomGroup at input time frame. " | ||
orbeckst marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"Grid for density could not be automatically" | ||
" generated. If this is expected, a user" | ||
" defined grid will need to be " | ||
"provided instead.") | ||
raise ValueError(errmsg) from err | ||
BINS = fixedwidth_bins(self._delta, smin, smax) | ||
arange = np.transpose(np.vstack((BINS['min'], BINS['max']))) | ||
bins = BINS['Nbins'] | ||
|
@@ -487,8 +506,8 @@ def _set_user_grid(gridcenter, xdim, ydim, zdim, smin, smax): | |
raise ValueError("xdim, ydim, and zdim must be numbers") from err | ||
|
||
# Set min/max by shifting by half the edge length of each dimension | ||
umin = gridcenter - xyzdim/2 | ||
umax = gridcenter + xyzdim/2 | ||
umin = gridcenter - xyzdim / 2 | ||
umax = gridcenter + xyzdim / 2 | ||
|
||
# Here we test if coords of selection fall outside of the defined grid | ||
# if this happens, we warn users they may want to resize their grids | ||
|
@@ -657,7 +676,7 @@ def __init__(self, *args, **kwargs): | |
|
||
parameters = kwargs.pop('parameters', {}) | ||
if (len(args) > 0 and isinstance(args[0], str) or | ||
isinstance(kwargs.get('grid', None), str)): | ||
isinstance(kwargs.get('grid', None), str)): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Looks bizzare to me. I would revert this. |
||
# try to be smart: when reading from a file then it is likely that | ||
# this is a density | ||
parameters.setdefault('isDensity', True) | ||
|
@@ -695,7 +714,8 @@ def _check_set_unit(self, u): | |
# all this unit crap should be a class... | ||
try: | ||
for unit_type, value in u.items(): | ||
if value is None: # check here, too iffy to use dictionary[None]=None | ||
# check here, too iffy to use dictionary[None]=None | ||
if value is None: | ||
self.units[unit_type] = None | ||
continue | ||
try: | ||
|
@@ -763,7 +783,8 @@ def convert_length(self, unit='Angstrom'): | |
""" | ||
if unit == self.units['length']: | ||
return | ||
cvnfact = units.get_conversion_factor('length', self.units['length'], unit) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe 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. |
||
cvnfact = units.get_conversion_factor( | ||
'length', self.units['length'], unit) | ||
self.edges = [x * cvnfact for x in self.edges] | ||
self.units['length'] = unit | ||
self._update() # needed to recalculate midpoints and origin | ||
|
@@ -814,8 +835,8 @@ def convert_density(self, unit='Angstrom'): | |
if unit == self.units['density']: | ||
return | ||
try: | ||
self.grid *= units.get_conversion_factor('density', | ||
self.units['density'], unit) | ||
self.grid *= units.get_conversion_factor( | ||
'density', self.units['density'], unit) | ||
except KeyError: | ||
errmsg = (f"The name of the unit ({unit} supplied) must be one " | ||
f"of:\n{units.conversion_factor['density'].keys()}") | ||
|
@@ -827,4 +848,4 @@ def __repr__(self): | |
grid_type = 'density' | ||
else: | ||
grid_type = 'histogram' | ||
return '<Density ' + grid_type + ' with ' + str(self.grid.shape) + ' bins>' | ||
return f"<Density {grid_type} with {self.grid.shape} bins>" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you test this line? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, perhaps I'll have to think about a test for this. I'd leave it alone, but it looks like autoPEP8 felt that the line was very long! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @orbeckst, I've removed this change. However, a check seems to be failing and I'm not sure if it is a concern.(?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test failure was down to a non-related RDKIT test (it's a flaky test that fails sometimes on CI), I've restarted the jobs, hopefully they should pass this time. |
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.
you also raise a
ValueError
– please document