-
Notifications
You must be signed in to change notification settings - Fork 17
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
load_collection: Clarification on temporal_extent #331
Comments
Hey @Ardweaden, good point. Quickly going through it for clarity. Here is the definition of the parameter and I tried to highlight the relevant parts: The Now the issue is that the same instance should be included and excluded, indeed. Strictly speaking, I'd say they are invalid right now and should probably be two distinct dates. Nevertheless, I see that people use the notion of providing the same date times pretty often. So we should probably consider mentioning that "including" beats "excluding" ;-) or something like that. What would be your preference? |
This was the suggestion from @mkadunc
I don't really have a preference, we just want to implement the standard as closely as possible to how it was intended so that we don't end up with unnecessary backend-specific quirks. I do think that if this is somewhat unclear, it would probably best to clarify it using the interpretation of VITO's backend, given it is by far the most advanced, |
For some context, here's the discussion that lead to the current definition: #7 A good argument here is that this follows ISO 8601 wrt the interpretation of time intervals. As such, I'd expect that this is also how libraries implement it and we probably shouldn't derive from it too far. Unfortunately, I can't find any information in 8601 about this special case yet. So following ISO 8601: the duration of giving the same start and end time would always be 0. All changes here would effectively break the existing definitions, which is not possible. What we could do is that we actually clarify the documentation. Should specifying the same start and end date result in an exception? Not very satisfying though. By the way, this also influences filter_temporal, load_result, and potentially aggregate_temporal and climatological_normal.
What does valid mean here? What does it result in? |
Of, course, this is what we were asking to begin with. The idea, however, was constructed in a way that it shouldn't violate the current wording of the process, but allows for time intervals with identical start/end.
True, but it sounds to me like it's the most consistent with the current wording. But the most practically useful would probably be
It means that e.g. the following doesn't raise an error but returns a valid result:
or
|
I think the "including beats excluding" is also compliant with ISO8601, but it may not exactly do what you expect here.
To add to what I wrote in the previous paragraph, this should only return data for the first (milli)second of the day. It seems that there's an implementation detail about VITOs data cubes that may actually return a "composite" of the full day though. I just learned about it today: https://discuss.eodc.eu/t/add-a-time-acquisition/256 So by just merging to the first second of the day, you actually get what you are looking for although the original acquisitions are captured throughout the day. Not really verified that, but it looks like this is true. |
Millisecond or second? If the interval bounaries are interpreted as instants, then the precision shouldn't matter and we should assume infinite precision. (i.e. we would only return data that are timestamped with the exact same UTC time, i.e. "2017" or "2017-01" or "2017-01-01" or "2017-01-01T00:00:00Z") Re ISO 8601 - a copy of the standard that I found has this to say about
|
Yes, I meant instants and agree with you. The "(milli)seconds" were referring to the "representation" here as in ISO8601 you usually provide second or millisecond precision in the textual representation. Interesting. Which copy/revision/version is this from @mkadunc? I'll also check our version again... |
The text is from https://www.sis.se/api/document/preview/905564/ |
FYI: it's a known issue that the VITO back-end indeed does not follow the [include, exclude] rules strictly: It's not trivial to change/fix the implementation because we don't want to break existing usage that depends on the current behavior, as noted above:
@jdries knows the history and context better, but I don't think we have a concrete plan of how to get out of this pickle, yet. Also as noted above, we work with day-composite time resolution for most of our internal collections, practically meaning that observations always have time |
I can't find a full-text of the ISO 8601 PDFs anymore, so the only source is @edzer in #7 for now. Asked for his source. Anyway, we can't really change the semantics in the processes completely. We may only clarify behavior that is ambiguous. The "including beats excluding" doesn't seem very useful though as it's still just an instance, e.g. ["2017", "2017"] would then simply be "2017-01-01T00:00:00.0000Z". That's not intuitive to users and I think they would expect something different than they get. So I think it would be better to let them know and throw an error in this case, telling them to increase the range. VITO would probably stay incompatible for now until we move to v2 or so. I don't really have a better solution for now. |
Thank you. If this decisions is final and everyone agrees, we'll implement it like that. But I'd ask for this clarification to be put into the definition of the |
This decision is not final, I'd still like hear the opinions of more implementors. Let me drag some more people in: |
Any progress on this? |
No, I did not hear any further feedback (e.g. from the people tagged above). |
In my opinion from the process description, When parsing the |
Just letting you know that we implemented in a very similar to what @clausmichele described, except we always subtract a millisecond. |
…ance in time must be after the first instance in time. #331
It seems the implementations tend more towards the approach that the two elements in the intervals should not be the same, so I created a PR that proposes this breaking change for v2.0.0: #331 |
|
* The temporal intervals must always be non-empty, i.e. the second instance in time must be after the first instance in time. #331 * Add uniqueItems, remove mention of 24 as the hour, remove temporal-interval subtype from climatological_normal (as it has inclusive upper boundaries) * Improved terminology * Update load_stac * Updates according to recent discussions #331 * Apply suggestions from code review Co-authored-by: Stefaan Lippens <soxofaan@users.noreply.github.com> * Remove timezone from times --------- Co-authored-by: Stefaan Lippens <soxofaan@users.noreply.github.com>
Process ID: load_collection
Describe the issue:
Documentation states:
Would e.g
["2017-01-01", "2017-01-01"]
or["2017-01-01T00:00:00Z", "2017-01-01T00:00:00Z"]
be valid or invalidtemporal_extent
s? If it's valid, how would["2017-01-01", "2017-01-01"]
be converted to a closed datetime interval?Proposed solution:
Add examples to clarify behaviour when values in
temporal_extent
are the same.Additional context:
We noticed VITO's backend considers aforementioned
temporal_extent
s valid.The text was updated successfully, but these errors were encountered: