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

EDR CoverageJSON conformance fixes: time axis and demo data #1814

Open
wants to merge 29 commits into
base: master
Choose a base branch
from

Conversation

pzaborowski
Copy link
Contributor

@pzaborowski pzaborowski commented Sep 20, 2024

Overview

changes to confirm to https://schemas.opengis.net/covjson/1.0/coveragejson.json

  • time axis changed to values (was interval)
  • time axis first in range as expected by https://github.com/Reading-eScience-Centre/leaflet-coverage and dependancies
  • parameter description as object - worked with JS visualisation but not validator and covjson playground
  • range dataType as float (was number and allowed are float, integer, string) - worked with JS visualisation but not validator and covjson playground

Other:

  • typo in edr/query.html template causing issues with first parameter layer

Changes shall solve issues using https://covjson.org libraries, also on pygeoapi demo site.

Related Issue / discussion

mainly #1833
but pull depends on this
#1782

Additional information

changes is not fully backward compatible with potential non-standard client using time as interval (which is breaking coveragejson-schema).

In addition to code changes, coads_sst.nc is fixed. Demo file had COADSX shifted west but not data which looks like this:
image

Dependency policy (RFC2)

  • I have ensured that this PR meets RFC2 requirements

Updates to public demo

Contributions and licensing

(as per https://github.com/geopython/pygeoapi/blob/master/CONTRIBUTING.md#contributions-and-licensing)

  • I'd like to contribute [feature X|bugfix Y|docs|something else] to pygeoapi. I confirm that my contributions to pygeoapi will be compatible with the pygeoapi license guidelines at the time of contribution
  • I have already previously agreed to the pygeoapi Contributions and Licensing Guidelines

pzaborowski and others added 25 commits August 12, 2024 11:32
Reading from query bbox and reversing the order would not work anyway if not followed by reversing data (which is not cheap and would not do on default. It is visible on the sample data tests/data/coads_sst.nc where the start-stop swapping turn the data upside down.
there is no sense in reading it from source file once again
1. time axis shall be 't' in covjson, for backward compliance it can be set with 'time_axis_covjson' provider variable. if time_axis_covjson is not set the value is read from time_field which has default value read from the source file.
2. time axis is now list of values according to specification and it is now properly rendered on the html view of the query
3. typo in query.html to fix error with first layer being not generated
1.integrated changes from axis fix
2. dataType is float according to schema https://schemas.opengis.net/covjson/1.0/coveragejson.json (number is not valid, float, integer, string are)
3. parameter description as object according to schema https://schemas.opengis.net/covjson/1.0/coveragejson.json
coards_sst.nc data was shifted around 180 deg
* Update compliance for OGC API - Processes

* Update introduction.rst
* fix queryables provider handling

* fix test
* Manage non-cf-compliant time dimension

* Manage datasets without a time dimension

* Allow reversed slices also for axes

* Convert also metadata to float64 for json output

* Use named temporary file to enable netcdf4 engine

* Make float64 conversion faster

* Add netcdf output to xarray provider

* Flake8 fixes

* Fix bug when no time axis in data

* Use new xarray interface

* Add test for zarr dataset without time dimension

* Avoid errors if missing long_name

* Manage zarr and netcdf output in the same way

* Revert "Manage zarr and netcdf output in the same way"

This reverts commit 0b09281.

* Revert "Add netcdf output to xarray provider"

This reverts commit 9f72bf7.
* Added ability for self-hosted token service to be specified.

* Update documentation to show the available parameters

* Update pygeoapi/provider/esri.py

Co-authored-by: Benjamin Webb <[email protected]>

* Update pygeoapi/provider/esri.py

Co-authored-by: Benjamin Webb <[email protected]>

* Update pygeoapi/provider/esri.py

Co-authored-by: Benjamin Webb <[email protected]>

* Update pygeoapi/provider/esri.py

Co-authored-by: Benjamin Webb <[email protected]>

* Update pygeoapi/provider/esri.py

* Update ogcapi-features.rst

---------

Co-authored-by: Benjamin Webb <[email protected]>
Co-authored-by: Tom Kralidis <[email protected]>
@tomkralidis tomkralidis requested a review from webb-ben October 24, 2024 17:42
@tomkralidis tomkralidis self-requested a review October 24, 2024 17:42
@tomkralidis tomkralidis added the bug Something isn't working label Oct 24, 2024
@tomkralidis tomkralidis added this to the 0.19.0 milestone Oct 24, 2024
Copy link
Member

@webb-ben webb-ben left a comment

Choose a reason for hiding this comment

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

Looks good. Please rebase!

Comment on lines +177 to +179
# assert time_dict['start'] == '2000-08-16T07:23:42.000000000'
# assert time_dict['stop'] == '2000-12-16T01:20:05.999999996'
# assert time_dict['num'] == 7
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# assert time_dict['start'] == '2000-08-16T07:23:42.000000000'
# assert time_dict['stop'] == '2000-12-16T01:20:05.999999996'
# assert time_dict['num'] == 7

Copy link
Member

Choose a reason for hiding this comment

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

This should be already addressed in upstream pygeoapi

Comment on lines +157 to +159
# assert time_dict['start'] == '2000-06-16T10:25:30.000000000'
# assert time_dict['stop'] == '2000-08-16T07:23:42.000000000'
# assert time_dict['num'] == 3
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# assert time_dict['start'] == '2000-06-16T10:25:30.000000000'
# assert time_dict['stop'] == '2000-08-16T07:23:42.000000000'
# assert time_dict['num'] == 3

Comment on lines +138 to +140
# assert time_dict['start'] == '2000-06-16T10:25:30.000000000'
# assert time_dict['stop'] == '2000-08-16T07:23:42.000000000'
# assert time_dict['num'] == 3
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# assert time_dict['start'] == '2000-06-16T10:25:30.000000000'
# assert time_dict['stop'] == '2000-08-16T07:23:42.000000000'
# assert time_dict['num'] == 3

Comment on lines +95 to +96
self.time_axis_covjson = provider_def.get('time_axis_covjson') \
or self.time_field
Copy link
Member

Choose a reason for hiding this comment

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

What is the point of adding this? Can we add a test case that uses this field. Suggest to potentially rename in pygeoapi, but will defer to @tomkralidis. This addition should also be noted in the docs (OACoverages and OAEDR

time dimension in configuration just for backward compatibility, it is expected in covjson to be 't'
@tomkralidis tomkralidis modified the milestones: 0.19.0, 0.20.0 Dec 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants