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

subset-user-input.ipynb: fix output for Thredds v5 #317

Closed
wants to merge 3 commits into from

Conversation

tlvu
Copy link
Contributor

@tlvu tlvu commented Jan 17, 2024

Only merge if bird-house/birdhouse-deploy#413 can be merged.

This is mostly to have a nice notebook diff.

15:47:13  ___ pavics-sdi-master/docs/source/notebooks/subset-user-input.ipynb::Cell 12 ___
15:47:13  Notebook cell execution failed
15:47:13  Cell 12: Cell outputs differ
15:47:13  
15:47:13  Input:
15:47:13  # gather data from the PAVICS data catalogue
15:47:13  catalog = "https://boreas.ouranos.ca/twitcher/ows/proxy/thredds/catalog/datasets/gridded_obs/catalog.xml"  # TEST_USE_PROD_DATA
15:47:13  
15:47:13  cat = TDSCatalog(catalog)
15:47:13  data = cat.datasets[0].access_urls["OPENDAP"]
15:47:13  data
15:47:13  
15:47:13  Traceback:
15:47:13   mismatch 'text/plain'
15:47:13  
15:47:13   assert reference_output == test_output failed:
15:47:13  
15:47:13    "'https://pav...rcan_v2.ncml/'" == "'https://pav...s/nrcan.ncml'"
15:47:13    Skipping 70 identical leading characters in diff, use -v to show
15:47:13    - _obs/nrcan.ncml'
15:47:13    + _obs/nrcan_v2.ncml'
15:47:13    ?           +++
15:47:13

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@tlvu
Copy link
Contributor Author

tlvu commented Jan 17, 2024

@tlogan2000 is nrcan_v2.ncml and nrcan.ncml equivalent?

@tlogan2000
Copy link
Contributor

@tlogan2000 is nrcan_v2.ncml and nrcan.ncml equivalent?

It's not the same data no

@tlvu
Copy link
Contributor Author

tlvu commented Feb 13, 2024

@tlogan2000 is nrcan_v2.ncml and nrcan.ncml equivalent?

It's not the same data no

So it is not expected that with the new Thredds v5, nrcan_v2.ncml is used instead of nrcan.ncml?

@tlogan2000
Copy link
Contributor

@tvlu THREDDS sorting of datasets seems to have changed between v4 and v5. Two options to fix.

  1. Leave as is and accept that THREDDS v5 finds nrcan_v1 in the [0] index of the catalogue search. The example is still valid in terms of operations (subsetting and calculations etc etc)

or

  1. changing the line
data = cat.datasets[0].access_urls["OPENDAP"]

to

data = cat.datasets[-1].access_urls["OPENDAP"]

should ensure that v2 is found instead of v1

@tlvu
Copy link
Contributor Author

tlvu commented Feb 21, 2024

Is there an access by name instead of by index? Otherwise, the day we have nrcan_v3.ncml this will change again !

Also, whether we choose cat.datasets[0] or cat.datasets[1] we can not merge immediately and have to wait until Thredds v5 go-live to merge, else it won't be backward compatible with the existing Thredds v4.

@tlogan2000
Copy link
Contributor

Is there an access by name instead of by index

Nothing built in but I change the code to search for nrcan_v2 should be future proof now

@tlvu
Copy link
Contributor Author

tlvu commented Feb 23, 2024

Nothing built in but I change the code to search for nrcan_v2 should be future proof now

Super, I'd do a "refresh" with Thredds v5 and test against our current Thredds v4. If it works, we can merge this immediately and be forward compatible with Thredds v5 !

@tlvu tlvu closed this Dec 2, 2024
@tlvu tlvu deleted the update-output-for-Thredds-v5 branch December 2, 2024 22:00
@tlvu tlvu restored the update-output-for-Thredds-v5 branch December 2, 2024 22:16
@tlvu
Copy link
Contributor Author

tlvu commented Dec 2, 2024

Opps, closed by mistake.

@tlvu
Copy link
Contributor Author

tlvu commented Dec 5, 2024

Replace by #342

@tlvu tlvu closed this Dec 5, 2024
@tlvu tlvu deleted the update-output-for-Thredds-v5 branch December 5, 2024 20:42
tlvu added a commit that referenced this pull request Dec 5, 2024
…s v4 and v5 (#342)

Original commit was from Travis

0a76235

Replaces PR #317

To avoid this error when we will upgrade to Thredds v5, while being
backward compatible with current Thredds v4:

```
15:47:13  ___ pavics-sdi-master/docs/source/notebooks/subset-user-input.ipynb::Cell 12 ___
15:47:13  Notebook cell execution failed
15:47:13  Cell 12: Cell outputs differ
15:47:13  
15:47:13  Input:
15:47:13  # gather data from the PAVICS data catalogue
15:47:13  catalog = "https://boreas.ouranos.ca/twitcher/ows/proxy/thredds/catalog/datasets/gridded_obs/catalog.xml"  # TEST_USE_PROD_DATA
15:47:13  
15:47:13  cat = TDSCatalog(catalog)
15:47:13  data = cat.datasets[0].access_urls["OPENDAP"]
15:47:13  data
15:47:13  
15:47:13  Traceback:
15:47:13   mismatch 'text/plain'
15:47:13  
15:47:13   assert reference_output == test_output failed:
15:47:13  
15:47:13    "'https://pav...rcan_v2.ncml/'" == "'https://pav...s/nrcan.ncml'"
15:47:13    Skipping 70 identical leading characters in diff, use -v to show
15:47:13    - _obs/nrcan.ncml'
15:47:13    + _obs/nrcan_v2.ncml'
15:47:13    ?           +++
15:47:13
```
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.

2 participants