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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

GLODAP v2.2016 ocean data cmorizer #2185

Merged
merged 13 commits into from Oct 11, 2021
Merged

GLODAP v2.2016 ocean data cmorizer #2185

merged 13 commits into from Oct 11, 2021

Conversation

tomaslovato
Copy link
Contributor

@tomaslovato tomaslovato commented May 31, 2021

Description

Although ESMs were already compared to inorganic carbon ocean inventories in some paper and also within CRESCENDO project, the cmorizer for GLODAP v2.2016 observations is still missing.

Using the same approach adopted for woa18 (#1812), I put together a specific cmorizer for this dataset within OBS6 that download raw data if needed and prepares yearly (Oyr) climatologies for dissolved inorganic carbon (dissic), total alkalinity (talk) and ph at insitu temperature (ph).


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.

New or updated data reformatting script


To help with the number of pull requests:

Closes #2342

@zklaus zklaus added this to the v2.4.0 milestone Jun 23, 2021
Copy link
Contributor

@valeriupredoi valeriupredoi left a comment

Choose a reason for hiding this comment

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

many thanks @tomaslovato 馃嵑 A couple minor suggestions from me 馃憤

esmvaltool/cmorizers/obs/cmorize_obs_glodap.py Outdated Show resolved Hide resolved
Copy link
Contributor

@remi-kazeroni remi-kazeroni left a comment

Choose a reason for hiding this comment

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

@tomaslovato, thanks for your contribution here. I have just tried this cmorizer but I cannot get it work because the automated download procedure leads to a corrupted file that cannot be process as an archive. Could you please provide another source to the data so that I could run the cmorizer?

I would not recommend spending too much time on the automated download part here because this will be revisited anyway when #1657 is in place. That PR will provide a framework to simply add downloader scripts that are independent from the cmorizers. Instead, I think it is good enough for this PR to provide instructions on how to download raw data in the header of the cmorizer.

Finally, note that a GLODAP data entry in the recipe_check_obs,yml is missing and we need it to accept cmorizers.

Copy link
Contributor

@remi-kazeroni remi-kazeroni left a comment

Choose a reason for hiding this comment

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

Thanks for your reply @tomaslovato. The cmorizer works fine for me and the data produced look fine. I'm not sure what changed since the last time I tried, perhaps the tarfile at the provider side? Could you please add an entry to the recipe_check_obs.yml? Then this PR is good to go imo.

doc/sphinx/source/input.rst Outdated Show resolved Hide resolved
esmvaltool/cmorizers/obs/utilities.py Show resolved Hide resolved
Copy link
Contributor

@remi-kazeroni remi-kazeroni left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @tomaslovato 馃憤 Everything looks good to me now. I added the data to the RAWOBS and OBS folders. Just a couple of minor things before we merge this:

  • Could you please handle the remaining merge conflicts
  • Could you please go through the PR check-list? Everything is good in this case but we normally ask the author of the PR to tick the boxes if the criteria are met.
  • @valeriupredoi: could you please tell us if your previous comments have been addressed? It would be great to have this in the next release.

@tomaslovato
Copy link
Contributor Author

@remi-kazeroni I merged with the main and solved the conflicts. I also went through the checklist and everything should be in place ...

@remi-kazeroni
Copy link
Contributor

@remi-kazeroni I merged with the main and solved the conflicts. I also went through the checklist and everything should be in place ...

Thanks @tomaslovato! I forgot to write earlier that we also normally expect authors of PRs to start their work by opening an issue describing the work that is planned... Would you mind doing so? Sorry for being so bureaucratic...
Once @valeriupredoi has approved the PR, we can merge this.

Copy link
Contributor

@valeriupredoi valeriupredoi left a comment

Choose a reason for hiding this comment

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

excellent work @tomaslovato 馃嵑

@valeriupredoi
Copy link
Contributor

@remi-kazeroni all good by me, man!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

create a cmorizer for GLODAP observational dataset
5 participants