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

Implement cams egg4 dataset (for co2) #26

Merged
merged 17 commits into from
Aug 23, 2023
Merged

Implement cams egg4 dataset (for co2) #26

merged 17 commits into from
Aug 23, 2023

Conversation

geek-yang
Copy link
Member

Adding support for CAMS EGG4 dataset in zampy.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@geek-yang geek-yang mentioned this pull request Aug 14, 2023
@geek-yang geek-yang marked this pull request as ready for review August 17, 2023 08:05
Copy link
Member Author

@geek-yang geek-yang left a comment

Choose a reason for hiding this comment

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

A demo notebook for downloading cams co2 data is available for reviewing this PR. (Note: ADS is very slow....the downloading query could take very long...be patient 😂 )

@BSchilperoort
Copy link
Contributor

BSchilperoort commented Aug 17, 2023

@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
Copy link
Member Author

@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.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Member

@SarahAlidoost SarahAlidoost left a comment

Choose a reason for hiding this comment

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

@geek-yang nice addition, thank you 👍

  • I think we can improve it by adding a recipe that downloads cams data.
  • 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.
  • How the test data is generated, perhaps it is better if we add a cell in the demo notebook that generates the test data.

@sonarcloud
Copy link

sonarcloud bot commented Aug 22, 2023

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
Copy link
Member Author

@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.

Copy link
Member

@SarahAlidoost SarahAlidoost left a comment

Choose a reason for hiding this comment

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

@geek-yang thank you for addressing my comments, changes look good 👍

@geek-yang geek-yang merged commit d59b018 into main Aug 23, 2023
14 checks passed
@geek-yang geek-yang deleted the implement-cams-co2 branch August 23, 2023 07:36
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.

3 participants