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

199 data request 20km regional projections for cordex cmip6 queensland future climate science #219

Draft
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

charles-turner-1
Copy link
Contributor

@charles-turner-1 charles-turner-1 commented Oct 14, 2024

@marc-white You added config/experiments/cmip6_ig45.yaml in this branch - I've separately added a config/metadata_sources/cordex_ig45.yaml which is doing largely the same thing but seemed to follow the other source conventions more closely. Are you able to confirm whether there's a convention I'm unaware of?

Closes #199

charles-turner-1 and others added 26 commits September 12, 2024 10:05
* Replaced a couple of try/excepts with .get in `src/access_nri_intake/catalog/translators.py`

* Updated a misleading docstring
likely to be necessary for passing around coordinates as well as
data variables as we begin to make coordinates indexable - I think
we'll begin to get confused about what belongs where.
   dataclasses that are fed into a dataclass holding all the
   coordinate and data variables from a netCDF file.
- Updated tests so that they are all passing: tests now expect
  to find coordinate variables from the netCDF files as well as
  data variables.
- Some minor changes to make code more readable - changed long
  tuples to dataclasses & dictionaries where possible.
…due to use of '|' type union syntax - might be worth consideration?

- Removed a couple of unused imports, cleaned up some comments
- Updated tests to better respect moving coordinate variables back into variables
- Moved mypy setup stuff into to 'pre-commit-config.yaml'
…gional-projections-for-cordex-cmip6-queensland-future-climate-science

Most interested in keeping typing hanging around for development speed.
…ex.yaml and config/metadata_sources/cordex-ig45/metadata.yaml
…al-projections-for-cordex-cmip6-queensland-future-climate-science
…-20km-regional-projections-for-cordex-cmip6-queensland-future-climate-science"

This reverts commit 27e460f, reversing
changes made to 0c96e28.
Copy link

codecov bot commented Oct 14, 2024

Codecov Report

Attention: Patch coverage is 98.55072% with 1 line in your changes missing coverage. Please review.

Project coverage is 96.89%. Comparing base (06a9d42) to head (b303840).
Report is 13 commits behind head on main.

Files with missing lines Patch % Lines
src/access_nri_intake/catalog/translators.py 98.55% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #219      +/-   ##
==========================================
- Coverage   96.95%   96.89%   -0.06%     
==========================================
  Files           9        9              
  Lines         623      644      +21     
==========================================
+ Hits          604      624      +20     
- Misses         19       20       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@marc-white marc-white self-requested a review October 14, 2024 05:09
Copy link
Collaborator

@marc-white marc-white left a comment

Choose a reason for hiding this comment

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

@charles-turner-1 there isn't a convention for naming the metadata sources that we're storing in the repository. This is a very recent change that was made to avoid repeating issues like #200. However, they're only reference copies, and the catalog itself should reference the metadata.yaml stored somewhere on /g/data. Therefore, you can really do what you like with this, so long as we stay internally consistent.

Comment on lines 9 to 14
frequency:
-
variable:
-
nominal_resolution:
-
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can just leave these as blank-blank, rather than a blank array.

Comment on lines 22 to 23
related_experiments:
-
Copy link
Collaborator

Choose a reason for hiding this comment

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

As above

Comment on lines 15 to 17
- metadata_yaml: /g/data/xp65/admin/intake/metadata/cmip6_ig45/metadata.yaml
path:
- /g/data/ig45/catalog/v2/esm/catalog.json
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is the path where the 'live' metadata.yaml is being stored, then I'd recommend the reference copy's name/path within the repo match, i.e. be cmip6_ig45.


sources:

- metadata_yaml: /g/data/xp65/admin/access-nri-intake-catalog/config/metadata_sources/cordex-ig45/metadata.yaml
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is a path where we want to be storing live YAML... see discussion on #200

Copy link
Collaborator

@marc-white marc-white left a comment

Choose a reason for hiding this comment

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

As it currently stands, it looks like you're going to load the same Intake-ESM catalogue into access-nri-intake-catalog twice, using two different translators and metadata.yaml files. I presume you wanted to use the cordex ones in the final merge?

  sentry)
- Updated cmip6.yaml as a different translator is required for Cordex
  experiments as main CMIP6 experiments.
@charles-turner-1 charles-turner-1 force-pushed the 199-data-request-20km-regional-projections-for-cordex-cmip6-queensland-future-climate-science branch from f9dbb95 to b303840 Compare October 14, 2024 08:17
@charles-turner-1
Copy link
Contributor Author

Yup - the changes to cmip6.yaml seem to go with the draft metadata.yaml you added. The cordex experiments needed a different translator, so I've separated them out - just hadn't cleaned up that file - everything should be sensible now.

I'm not 100% sure I understand the discussion around YAML paths fully - either that or it appears we need to update a bunch of paths in config/*.yaml?

@marc-white
Copy link
Collaborator

@charles-turner-1 There's a few things about YAML paths:

  • Typically, the metadata.yaml lives with the data. However, there are some "historical" experiments for which we don't have write access to their data area, so we have to store the metadata.yaml within our own xp65 project. Where in xp65 was debated, given there is also an access-nri-intake-catalog checkout in there somewhere we don't want to fiddle with/be dependent on in production.
  • There were some experiments that we were already holding the metadata for in xp65, but it accidentally got deleted. That's why we're holding all the metadata for the live catalog in a single place (/g/data/xp65/admin/intake/metadata) to try and stop that happening again.
  • Separately, @rbeucher asked that we start holding a reference copy of new experiment metadata.yamls within the access-nri-intake-catalog repository. This isn't for production use; it's just for reference in case the above problem happens again (and I guess might be handy for testing too, now that I think about it).

@charles-turner-1 charles-turner-1 changed the base branch from main to 186-return-type October 15, 2024 03:22
@charles-turner-1 charles-turner-1 changed the base branch from 186-return-type to main October 15, 2024 03:22
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.

[DATA REQUEST] 20km regional projections for CORDEX-CMIP6 from the Queensland Future Climate Science Program
2 participants