Skip to content
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.

Sign up for GitHub

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

Jump to bottom

FIX: Fixing time selection when precision is different #344

Merged
merged 4 commits into from
Dec 2, 2019
Merged

FIX: Fixing time selection when precision is different #344

merged 4 commits into from
Dec 2, 2019

Conversation

mpu-creare
Copy link
Contributor

Consider:

c1 = Coordinates(['2012-05-19'], ['time'])
c2 = Coordinates(['2012-05-19T12:00:00'], ['time'])
c12 = c1.intersect(c2)
c21 = c2.intersect(c1)

With this PR, I propose that c12 and c21 should contain an intersection.

@mpu-creare
FIX: Fixing time selection when the precision of the self and other a…
1733dc1 
…re not the same. By default, the lowest precision should be used to determine if time data intersects.
@mpu-creare mpu-creare self-assigned this Nov 27, 2019
@mpu-creare mpu-creare added the bug Something isn't working label Nov 27, 2019
@mpu-creare
Copy link
Contributor Author

Note, this change is required to have the SMAP EGI node work when requesting data for a single date.

@coveralls
Copy link

coveralls commented Nov 27, 2019 •
edited
Loading

Coverage Status

Coverage decreased (-0.01%) to 81.991% when pulling 6b22224 on feature/time-select-precision into 8c4968e on develop.

@jmilloy
Copy link
Collaborator

jmilloy commented Nov 27, 2019

I want to look at this more closely later this afternoon. I think this makes sense. Leaving myself a note to double check - I think we may only want the lower precision in one direction, that is, 2019-01-01 would intersect 2019-01 but not 2019-01-01 12:00:00.

jmilloy
jmilloy requested changes Dec 2, 2019
View reviewed changes
Copy link
Collaborator

@jmilloy jmilloy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Converting my_bounds to the precision of the input bounds makes sense to me in general. I think that's what a naive user would expect using the select method.

podpac/core/coordinates/coordinates1d.py Outdated Show resolved Hide resolved
podpac/core/coordinates/coordinates1d.py Outdated Show resolved Hide resolved
podpac/core/coordinates/coordinates1d.py Outdated Show resolved Hide resolved
podpac/core/coordinates/uniform_coordinates1d.py Outdated Show resolved Hide resolved
@mpu-creare
FIX: Updates based on review.
8f2a0aa 
* factored out the lowering of the precision to a function
* Only do this selection when outer==True for higher precision other (compared to inner)
* Check for self.dtype instead of self.name
* Check input bound dtypes and throw TypeError if anything is not a np.datetime64
@mpu-creare
Copy link
Contributor Author

Ok @jmilloy . I made those changes. Please merge when you're happy.

@jmilloy
Always check the select bounds type. Adds a short-circuit if the coor…
6b22224 
…dinates are empty (and dtype is None), and removes the (redundant) bounds type checking in the private _select method.
jmilloy
jmilloy approved these changes Dec 2, 2019
View reviewed changes
Copy link
Collaborator

@jmilloy jmilloy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed another tweak that always checks the bounds dtype. Looks good.

@jmilloy jmilloy merged commit a74f633 into develop Dec 2, 2019
@jmilloy jmilloy deleted the feature/time-select-precision branch December 2, 2019 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmilloy jmilloy jmilloy approved these changes

@mlshapiro mlshapiro Awaiting requested review from mlshapiro

Assignees

@mpu-creare mpu-creare

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mpu-creare @coveralls @jmilloy