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 NLDAS model choice (no breaking changes) #2

Conversation

LucRSquared
Copy link
Contributor

@LucRSquared LucRSquared commented Apr 20, 2023

@LucRSquared
Copy link
Contributor Author

This is my first ever contribution to a repository that's not my own. I really tried my best to follow the contributing guidelines.

There was a small issue with the conda environment where pynldas2 could not be imported because of rasterio. According to this question https://gis.stackexchange.com/questions/417733/unable-to-import-python-rasterio-package-even-though-it-is-installed importing osgeo beforehand (even unused) fixed the issue.

There are no breaking changes for people who already use the library.

The basic idea was to create another dictionary containing the variables for NLDAS 2.0 and update the utility functions to handle that. The output from the API is slightly different so I had to modify _txt2dfand _txt2da based on which model was used.

Obviously feel free to edit things to your liking!

pynldas2/pynldas2.py Outdated Show resolved Hide resolved
pynldas2/pynldas2.py Outdated Show resolved Hide resolved
@cheginit
Copy link
Contributor

This is my first ever contribution to a repository that's not my own. I really tried my best to follow the contributing guidelines.

Congratulation on your first PR to an open source project, hopefully, one of many! Overall, you did a good job. I just added some comments to match the overall structure of HyRiver packages.

There was a small issue with the conda environment where pynldas2 could not be imported because of rasterio. According to this question https://gis.stackexchange.com/questions/417733/unable-to-import-python-rasterio-package-even-though-it-is-installed importing osgeo beforehand (even unused) fixed the issue.

That's strange, never had this issue. Will look into it. For now, we can keep it.

There are no breaking changes for people who already use the library.

That's the best to do it.

The basic idea was to create another dictionary containing the variables for NLDAS 2.0 and update the utility functions to handle that. The output from the API is slightly different so I had to modify _txt2dfand _txt2da based on which model was used.

Overall, the implementation is good, but there are some user interface issues that can help make it much easier to work with.

@LucRSquared
Copy link
Contributor Author

Thank you for the quick feedback! I will make the adjustments you suggested. I'm not sure about the procedure. I assume I can add a new commit to the branch? (I'm always worried to make mistakes with Git)

@cheginit
Copy link
Contributor

Yes, just commit and push your changes to the same branch that you started the PR with, and they will be automatically shown here.

@LucRSquared
Copy link
Contributor Author

This should do it! Sorry for the multiple commits, I had left a test notebook in there.

Somehow my local environment setup was a bit weird as I had the main version of pynldas2 in my dev environment and it would not import the one I was currently working on

I had to recreate the environment by removing the git+https://github.com/hyriver/pynldas2.git line from the yml file and then do a local install pip install -e . Clearly I'm not a pro with working with packages but this is a good learning experience.

Also, the API was a bit wonky today for the netcdf and would only work for dates in 2022, I checked using the normal link:

https://hydro1.gesdisc.eosdis.nasa.gov/daac-bin/access/timeseries.cgi?variable=NLDAS2:NLDAS_FORA0125_H_v2.0:Rainf&location=GEOM:POINT(-89.0,%2034.0)&type=asc2&startDate=2022-01-01T00&endDate=2022-01-07T00 (works)

https://hydro1.gesdisc.eosdis.nasa.gov/daac-bin/access/timeseries.cgi?variable=NLDAS2:NLDAS_FORA0125_H_v2.0:Rainf&location=GEOM:POINT(-89.0,%2034.0)&type=asc2&startDate=2021-01-01T00&endDate=2021-01-07T00 (does not work)

While yesterday it was working fine so maybe it's just a temporary server issue. I updated the test file according when testing the netcdf source.

@sonarcloud
Copy link

sonarcloud bot commented Apr 21, 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

No Coverage information No Coverage information
0.0% 0.0% Duplication

@cheginit
Copy link
Contributor

Having multiple commits are normal and even recommended for commit that have different purposes. So, don't worry about it. The modifications look good to me, thanks for taking the time and finalizing the PR!

Regarding the issue with the environment, you didn't have to remove the line related to the main pynldas2 git repo, technically, you could have just reinstalled it by doing pip install . --no-deps while being in your own branch and the correct environment.

@cheginit cheginit merged commit 52872ee into hyriver:main Apr 21, 2023
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