-
Notifications
You must be signed in to change notification settings - Fork 128
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
Download and formatting of NOAA marine boundary layer data for CH4 (NOAA-MBL-CH4) #3301
Conversation
Note, that the tests fail, because the necessary cmor table for the variable
I could make a pull request concerning this in the ESMValCore, however I also found the CMIP6 variable ch4global, which could fit at least to the NOAA dataset as well. I am testing that. |
Hey @FranziskaWinterstein, |
@hb326 Thank you! Yes, that is fine. 1e-09 is more common anyway I think. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@FranziskaWinterstein: Thanks for the formatter and cmorizer! I successfully downladed and formatted the data, tested the file with the recipe, and also looked at the data to check if it was in the correct units.
All works fine, the units look good, and everything runs smoothly with the recently merged "ch4s" entry to the custom cmor tables.
I have found a few minor things that I commented on, but nothing major. 👍
ch4s: | ||
additional_datasets: | ||
- {dataset: NOAA-MBL-CH4, project: OBS, mip: Amon, type: atmos, version: 1.0, tier: 2} | ||
scripts: null | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@FranziskaWinterstein: did you leave the years out here because the dataset is delivered in one file anyway? With not asking for a specific time period, this is recipe is flexible enough to just read whatever is there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I am unsure what would happen if the downloaded .csv file covers different years. Would you recommend to add years of the current available period?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would probably add the years just because the other datasets in the recipe have a specified time period. Not sure if we have a clear rule here... @schlunma, @valeriupredoi, any recommendations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@valeriupredoi: any recommendations here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would indeed - it's good to have it, if the user wants only a few years rather than download/cmorize the entire data span. In this case my review below becomes void since I am suggesting removing exactly these optional arguments (start/end times), if they are not used 👍
There was a problem hiding this 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, @FranziskaWinterstein! The code looks good to me and all works fine: downloader, formatter, recipe_check_obs. I agree with @hb326's suggestions and added a couple of very minor points you may want to consider.
There are a few failing tests but nothing that can be addressed (for now):
- the remaining Codacy issues can be ignored
- the CircleCI run_tests fails because the recently added custom table (
ch4s
) is not yet part of a released version of ESMValCore. The test will pass once a new ESMValCore release will be made. This PR could only be merged after that. Thus, I added the "requires a new ESMValCore release" label.
I can pre-approve on the technical side 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, @FranziskaWinterstein! Thanks for implementing the changes. I think the pull request is not ready to be merged (if we get the failing tests to pass...).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couple minor points from me. Many thanks @FranziskaWinterstein - the tests run with the latest development ESMValCore so all's OK on that front 🍺
EDIT: I decided not to stress on about unused arguments since these are endemic to a lot of cmorizers, and we should check for the need and make them optional in the main cmorizer module first; but clearly, that needs to be done in a dedicated PR, not here, and not in @schlunma 's PR (sorry for busting your balls on this, guys) 🍺
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
squashing my stale review - all fine, and many thanks, Franziska! Ready for merge, unless you want to implement a start time/end time optionality 🍺
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blast! Re-blocking this due to the failed test - apologies 👍 Needing ESMValCore 2.10 that includes the cmor table for ch4s plopped by Birgit in ESMValGroup/ESMValCore#2168
@valeriupredoi Thank you for taking care of this pull request. I am currently a little bit lost. Do you need me to do anything? |
@FranziskaWinterstein no not at all, I just put a temporary blocker on it so one doesn't accidentally merge it until we have a new ESMValCore that includes ESMValGroup/ESMValCore#2168 (sorry it took me a few minutes to dig out the relevant PR that was merged in Core). All good by me otherwise 🍺 |
The review block was put in place to wait for the core release. That has happened and the tests do indeed work.
…OAA-MBL-CH4) (#3301) Co-authored-by: Birgit Hassler <33543691+hb326@users.noreply.github.com> Co-authored-by: Valeriu Predoi <valeriu.predoi@gmail.com> Co-authored-by: Klaus Zimmermann <klaus.zimmermann@smhi.se>
Description
This adds download and formatting scripts to import NOAA marine boundary layer (MBL) CH4 data.
It is downloading the provided .csv table provided here. It formats the .csv file into a netcdf file, adds dummy latitude and longitude (since it is a global value) and fixes metadata.
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