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

Add ocean variable to the ACCESS Live CMORiser #2601

Open
wants to merge 42 commits into
base: main
Choose a base branch
from

Conversation

rbeucher
Copy link
Contributor

@rbeucher rbeucher commented Dec 2, 2024

Description

Adds Ocean variable to the ACCESS Live CMORIser


Before you get started

Checklist

It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.


To help with the number pull requests:

@rbeucher rbeucher force-pushed the add-ocean-variable-access-live-cmoriser branch 2 times, most recently from eac1810 to afe5b6d Compare December 2, 2024 09:18
@ESMValGroup ESMValGroup deleted a comment from codecov bot Dec 2, 2024
Copy link

codecov bot commented Dec 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.70%. Comparing base (8b4a63a) to head (ea1734b).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2601      +/-   ##
==========================================
+ Coverage   94.68%   94.70%   +0.01%     
==========================================
  Files         251      251              
  Lines       14372    14426      +54     
==========================================
+ Hits        13608    13662      +54     
  Misses        764      764              

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

@rbeucher
Copy link
Contributor Author

rbeucher commented Dec 2, 2024

@rhaegar325 please check that I have not introduced any issues.
@flicj191. This is originally @rhaegar325's work. Please give it a try.

@rhaegar325
Copy link
Contributor

HI, @valeriupredoi , this pr is about a new version of access on-the-fly cmoriser. we add some extra function to make it compatible with two ocean variable. could you please review and merge it if there is no conflict.

@rbeucher rbeucher requested a review from flicj191 December 3, 2024 03:23
@valeriupredoi
Copy link
Contributor

many thanks folks, since @rbeucher assigned @flicj191 then I'll wait for her expert review then I'll have an overview looks and merge 👍

Copy link

@flicj191 flicj191 left a comment

Choose a reason for hiding this comment

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

Thanks @rhaegar325 I was able to run a simple recipe and find the data. I've added comments to documentation.
Does still run very slow, it looks like it has to read from each file the date range as the file name (eg.ocean_month.nc-20121231) doesn't have the date surrounded by '_', '-', '.' as in this dataset part:
https://github.com/ESMValGroup/ESMValCore/blob/ea1734b16899881e7436da83f1eadadb0fc9a788/esmvalcore/local.py#L77C1-L82C34
maybe can add to here? @valeriupredoi @rbeucher
not sure where's best to constrain reading specific variable from the file if that would improve? as each file might have like ~200cubes

@@ -593,7 +593,7 @@ Key Description Default value if not
``modeling_realm`` Realm attribute include `atm`, `ice` No default (needs to be
and `oce` specified in extra facets or
recipe if default DRS is used)
```special_attr`` A special attribute in the filename No default
``freq_attribute`` A special attribute in the filename No default
Copy link

Choose a reason for hiding this comment

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

As we are changing special_attr to freq_attribute we need to edit above as well in lines 576 and 562. Maybe also add for ocean directories and and files like line 561 and 562
'{dataset}/{sub_dataset}/{exp}/{modeling_realm}'
'ocean_{freq_attribute}.nc-*'
and also add to the list of supported variables for ocean (ln 556/557)

Copy link

Choose a reason for hiding this comment

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

this empty file can be removed

@rhaegar325
Copy link
Contributor

rhaegar325 commented Dec 5, 2024

Thanks @flicj191 for your review, I will remove the empty file and fix the doc. for the file-reading part, it is a problem and bother me for a long time, I think your suggestion could be a solution for this.

@rhaegar325
Copy link
Contributor

Hi, @valeriupredoi, it seems that I don't have write access to this repo since I can not push my local branch, could you please give me the write permission so that I could manage the branch of this pr?

@flicj191
Copy link

flicj191 commented Dec 9, 2024

Thanks @rhaegar325 I was able to run a simple recipe and find the data. I've added comments to documentation. Does still run very slow, it looks like it has to read from each file the date range as the file name (eg.ocean_month.nc-20121231) doesn't have the date surrounded by '_', '-', '.' as in this dataset part: https://github.com/ESMValGroup/ESMValCore/blob/ea1734b16899881e7436da83f1eadadb0fc9a788/esmvalcore/local.py#L77C1-L82C34 maybe can add to here? @valeriupredoi @rbeucher not sure where's best to constrain reading specific variable from the file if that would improve? as each file might have like ~200cubes

I have edited the local.py so it can find the right files by date without loading, though not sure if this is the best was and the affect on other files. Will have to find the ruff format error it added, sorry!

Having a look at the test dataset (- {project: ACCESS, dataset: ACCESS-ESM1-5, mip: Omon, sub_dataset: HI-CN-05, exp: history, freq_attribute: month, start_year: 2000, end_year: 2005})
The raw data has latitude and longitude as both dimension coordinate and auxiliary coordinate, so both 1D and 2D. This one could be a regular grid so maybe to fix we can remove the auxiliary coordinates? we may need to check with the modellers?
Otherwise, we should also add the areacello variable to be added as a 'supplementary variable' so that it can do area_statistics.

@rbeucher
Copy link
Contributor Author

rbeucher commented Dec 9, 2024

Thanks for your help with this @flicj191

@rhaegar325
Copy link
Contributor

Really appreciate for your help and your advice @flicj191

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.

4 participants