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

Added ERA5 support for initialization #99

Merged
merged 7 commits into from
Sep 6, 2023

Conversation

syedhamidali
Copy link
Contributor

@syedhamidali syedhamidali commented Sep 5, 2023

Replaced ERA-interim successfully with ERA-5, and It is working fine.
While running flake8 tests manually, I changed some line spacing in other functions as well (not any code or logic), but the linting workflow on GitHub is somehow failing, you might wanna take a look.
The function is named as make_initialization_from_era5
Thanks!

@rcjackson
Copy link
Collaborator

Hamid, could you please install precommit in your environment:

https://pre-commit.com/

Then run the pre-commit hooks over all files and commit the changes that are made to this PR. The instructions for installing precommit and running the hooks are at the link above.

@syedhamidali
Copy link
Contributor Author

Bobby,
Done! Check out commit 4

@rcjackson
Copy link
Collaborator

rcjackson commented Sep 5, 2023

Very nice! Linting passes now! One other thing that needs to be done is that the unit test for era_interim also needs to be updated:
FAILED pydda/tests/test_ecmwf.py::test_era_initialization - AttributeError: module 'pydda.initialization' has no attribute 'make_initialization_from_era_interim'

The file pydda/tests/test_ecmwf.py contains this unit test. I would update it to match the same time period and location, but for ERA5.

I am fine with ERA-Interim support being removed as ERA5 has replaced ERA-Interim.

@syedhamidali
Copy link
Contributor Author

Do you want me to work on tests as well, I could probably give it a try.

@rcjackson
Copy link
Collaborator

rcjackson commented Sep 5, 2023 via email

@syedhamidali
Copy link
Contributor Author

syedhamidali commented Sep 5, 2023

I did make some changes in unit tests, but there is some issue with absolute tolerance parameter. Since, I added the era5 file for Grid, you may have to redifne these tolerances

@rcjackson rcjackson merged commit 1deff71 into openradar:main Sep 6, 2023
9 checks passed
@rcjackson
Copy link
Collaborator

All tests have passed! Thank you @syedhamidali for this great contribution!

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.

2 participants