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.

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

Fix timezones in feat info #958

Merged
merged 5 commits into from
Nov 30, 2023
Merged

Fix timezones in feat info #958

merged 5 commits into from
Nov 30, 2023

Conversation

SpacemanPaul
Copy link
Contributor

@SpacemanPaul SpacemanPaul commented Nov 12, 2023

  1. Remove imports from future. It's all Python 3.x nowadays
  2. Update all license headers
  3. Simple attempted fix for feature-info timezone-handling issue reported on ODC Slack (in data.py)

Me: I think I might be able to see the problem - are the underlying datasets exposed in an Explorer instance I can take a look at?
User: in Explorer and cubedash, I have had to specifically set Chilean timezone because scenes were showing on the wrong days… for the same issue: https://explorer.datacubechile.cl/products/s2_l2a
Me: OWS is assuming that dates in the ODC index all have UTC timezones (which is standard practice in DEAustralia and DEAfrica, but not required by ODC). If this isn't true for your data, it would explain what we are seeing - and yes, it is an OWS issue. (edited)
User: So Explorer now shows data on the “correct” days. Before that, data would be split across days because it was only considering the day part of the date, not the time. In reality, all of this data is collected on the same “day” because it is during the day. The problem lies in the treatment of a “day” but then subtracting time.


📚 Documentation preview 📚: https://datacube-ows--958.org.readthedocs.build/en/958/

Copy link

codecov bot commented Nov 13, 2023

Codecov Report

Merging #958 (7e01824) into master (80988e8) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 7e01824 differs from pull request most recent head 96fd7c2. Consider uploading reports for the commit 96fd7c2 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #958      +/-   ##
==========================================
- Coverage   93.72%   93.71%   -0.02%     
==========================================
  Files          43       43              
  Lines        6489     6478      -11     
==========================================
- Hits         6082     6071      -11     
  Misses        407      407              
Files Coverage Δ
datacube_ows/__init__.py 100.00% <ø> (ø)
datacube_ows/band_utils.py 99.32% <ø> (ø)
datacube_ows/cfg_parser_impl.py 98.65% <ø> (ø)
datacube_ows/config_toolkit.py 100.00% <ø> (ø)
datacube_ows/config_utils.py 95.83% <ø> (ø)
datacube_ows/cube_pool.py 100.00% <ø> (ø)
datacube_ows/data.py 83.60% <100.00%> (-0.03%) ⬇️
datacube_ows/gunicorn_config.py 0.00% <ø> (ø)
datacube_ows/legend_generator.py 100.00% <ø> (ø)
datacube_ows/legend_utils.py 94.11% <ø> (ø)
... and 33 more

@SpacemanPaul SpacemanPaul marked this pull request as ready for review November 13, 2023 00:07
@@ -845,7 +843,7 @@ def feature_info(args):
if params.product.time_resolution.is_summary():
date_info["time"] = ds.time.begin.strftime("%Y-%m-%d")
else:
date_info["time"] = dataset_center_time(ds).strftime("%Y-%m-%d %H:%M:%S UTC")
date_info["time"] = dataset_center_time(ds).strftime("%Y-%m-%d %H:%M:%S %Z")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the actual bug fix.

@JonDHo
Copy link

JonDHo commented Nov 14, 2023

Sorry this has taken me a few days to test, but I had a few issues with out-of-date Helm API versions. I have now built and deployed a new version of datacube-ows and this fix didn't work. The reason is that I only want to show users dates, not datetimes, so params.product.time_resolution.is_summary() (L843) will return True (I am using "time_resolution": "solar").

I think the L843 fix might still be useful, but it looks like L844 is the issue in my case.

For additional reference, see:

@SpacemanPaul
Copy link
Contributor Author

SpacemanPaul commented Nov 14, 2023

There are three allowed values for "time_resolution": "solar", "summary" and "subday". Of these, only "summary" returns true from .is_summary(). If you are using "time_resolution": "solar" is_summary() will return False.

See the documentation for time_resolution

The time portion is suppressed for "summary" time resolution as it already assumed to always be 00:00:00-UTC (and other things will break if it isn't). For "solar" time resolution, the "time" field in GetFeatureInfo displays the capture time as it is recorded in the index (or at least that's the idea - before this bug fix it added "UTC" on the end even if the capture time was not indexed as UTC.) (although I'm not sure why it's still showing as UTC - wasn't the data indexed with Chile timezone UTC-3?)

You can write a user function to add e.g. a "capture_day" field to returned feature info JSON, but you can't suppress the time part of the dataset time if it exists in the index.

The documentation for adding custom fields to feature into is here

But there is a parallel bug in the update_ranges code that is causing the "advertised" solar dates to be off by one where the capture date is not indexed in UTC - I'll fix that now.

@SpacemanPaul
Copy link
Contributor Author

Can you update with the above commit and re-run datacube-ows-update (and wait for the cache to refresh) and retest? Thanks.

@JonDHo
Copy link

JonDHo commented Nov 14, 2023

Sorry yes, I misread. I will retest now. My update takes a bit over an hour (is that about normal with lots of data?), but will report back once it is done.

@JonDHo
Copy link

JonDHo commented Nov 14, 2023

ok, seems that this didn't fix it, but I don't quite understand why. I re-ran the update and cleared any caches (I think), but it is a bit difficult to test and debug on my end because I don't have a dev environment set up and it takes too long to process. I did previously have a dev environment, so I can see if I can get it back up and running...

@SpacemanPaul
Copy link
Contributor Author

My update takes a bit over an hour (is that about normal with lots of data?), but will report back once it is done.

datacube-ows-update --views is slow on large indexes - an hour seems reasonable. But this is only needed when adding new data.

datacube-ows-update is MUCH faster (less than a minute even on large indexes). This is all you need when updating to handle bugfixes in code or configuration changes (as in this case). (You also need to run it after running --views)

@JonDHo
Copy link

JonDHo commented Nov 14, 2023

ok, thanks. I was just running the same job that I ran for adding new data and assumed that it was needed for this, but good to know that I only need the basic update.

@SpacemanPaul
Copy link
Contributor Author

I haven't heard back from @JonDHo so I'm not sure if this is a complete fix, but it's definitely an improvement, so let's get it merged.

@JonDHo
Copy link

JonDHo commented Nov 30, 2023

Sorry, I haven’t managed to look deeper into it yet. The last fix didn’t fully resolve the issue but I can see that it is an improvement. I need to dig into the DB to try to work out whether it is at that level, in the ows layer or Terria.

Copy link
Member

@omad omad left a comment

Choose a reason for hiding this comment

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

It was in a different context (on web pages), but I didn't think Copyright notices required regularly updated dates.

@omad omad merged commit 20717bb into master Nov 30, 2023
12 checks passed
@omad omad deleted the fix_timezones_in_feat_info branch November 30, 2023 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants