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
DAS-2232 -small functions added to support the main solution in the t… #16
DAS-2232 -small functions added to support the main solution in the t… #16
Changes from 6 commits
5f8d987
14a634e
ca881d1
36a3c40
b2b30c5
51b110c
73390fd
9c22bfd
d972777
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.
Here is where we need the additional:
(excuse the pidgeon python)
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.
Maybe I will include it as a comment till we make the configuration 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.
I strongly prefer what we have now. Also, the function contents exactly matches the function name.
Also, the snippet supplied above is not correct Python code, so it's hard to know for sure what you are trying to achieve. Trying to decompose that snippet:
VarInfoFromDmr
instance does not have any of the listed dimensions as variables.and not []
is a no-op. It will always evaluate toTrue
, because it is being evaluated in isolation, and you are asking if an empty list is "falsy", which it is.While I don't like this approach, I think what you are trying to suggest would be more like:
If this was to be augmented in such a way, I would recommend breaking this check out into it's own function, because the set comprehension will become very hard to read.
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 can see splitting out the function to clarify the code and document the comprehension.
I'm less clear on forcing the upstream code to use this code without the additional check, and then add in the call to the new function in every usage. That additional check is now essential to ensure the case of OPeNDAP creating "empty" dimensions does not allow this check, by itself, to succeed. And of course, OPeNDAP's "empty" dimensions is pretty much always going to be the case.
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 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.
function updated and unit tests added - d972777
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 think I asked this on the other PR - should this check be mutually exclusive to the other checks in the longitude or latitude blocks, or should it be done in addition to those checks? Right now, if you have a fill value, you are only checking for where the coordinate is not fill, and not considering your other checks. (I tend to prefer this first check, but wanted to confirm what the logic was intended to be)
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.
right. if we have a fill value - we use that to check for validity of the data and if that is not available we check for the geo extent range. I guess we can check for it even if the fill value is provided - in case the coordinate data itself is bad data.
if we check the lat/lon valid range first, the check for fill does become redundant..the fill value would definitely be outside that range,...
I guess @autydp can weigh in
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 would check the coordinates regardless, and let the fill-value be outside that range. It simplifies the code and those checks need to happen.
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.
updated - 51b110c
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 reworking this so that the fill value check and the latitude/longitude range checks can both happen.
I think the
coordinate.is_latitude()
andcoordinate.is_longitude()
checks could benefit from somenumpy
magic, rather than looping individually through each element. I think what you could use is the element-wise-and, which can be either written as&
ornp.logical_and
. You could do something like:Note, in the snippet above, I've also changed the first check from
if coordinate_fill
toif coordinate_fill is not None
. That's pretty important, as zero could be a valid fill value, but ifcoordinate_fill = 0
, then this check will evaluate toFalse
.Ultimately, I think the conditions you have now are equivalent to this, just maybe not as efficient. So the only thing I'd definitely like to see changed is that first
if coordinate_fill
condition, so that it's not considering of fill value of 0 to be non-fill.