search

EcoExtreML / zampy

Tool for downloading Land Surface Model input data
https://zampy.readthedocs.io/
Apache License 2.0
1 stars 0 forks source link

Implement cams egg4 dataset (for co2) #26

Closed geek-yang closed 1 year ago

geek-yang commented 1 year ago

Adding support for CAMS EGG4 dataset in zampy.

review-notebook-app[bot] commented 1 year ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

BSchilperoort commented 1 year ago

@geek-yang can you update the documentation as well? :)

I just realize now I did not do that with the PrismDEM dataset haha.

geek-yang commented 1 year ago

@geek-yang can you update the documentation as well? :)

I just realize now I did not do that with the PrismDEM dataset haha.

No problem! I will take care of it.

sonarcloud[bot] commented 1 year ago

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

92.9% 92.9% Coverage
0.0% 0.0% Duplication

geek-yang commented 1 year ago

@geek-yang nice addition, thank you 👍

Thanks a lot for your review @SarahAlidoost.

  • I think we can improve it by adding a recipe that downloads cams data.

An example recipe was added by Bart in the doc: https://github.com/EcoExtreML/zampy/blob/implement-cams-co2/docs/using_zampy.md#formulating-a-recipe

To avoid duplication, I simply add cams dataset there. I think later when we update both readme.md and the documentation, we can include more information.

  • Also, I still don't know which cams dataset can be downloaded using zampy. So, some clarifications should be added to the documentation. Because model_level=60 and step are hardcoded, please see my comments.

I add more to the documentation about these hardcoded parameters and the name of the target dataset (CAMS EGG4). Thanks for the suggestion!

  • How the test data is generated, perhaps it is better if we add a cell in the demo notebook that generates the test data.

After thinking about it, I feel that to add the generation of the test data to the demo notebook is not proper. The test data is designed specifically for unit tests. It could neither reflect what real data looks like nor let the user get a taste of all the dataset related methods shown in the notebook. While these demo notebooks are design as tutorials for users (and also developers) to get familiar with zampy API and to download real data and try the following steps (incl. ingest, load and convert). If we put them together the user might get a feeling that they could test all zampy methods with the test data and without downloading real data (which of course is not true).

I think eventually we will move the test data generation script to zampy and add a proper section in the doc showing how to use this script to generate test data. So I just made an issue (#31) and I suggest we could work on it in another PR.