removed extra argument given #78
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
**What's new:** - Fixed issue with Latitude above 65 °N, eanling PET estimation for high-latitude regions pyet-org#70 - Added examples using CMPI data, and an example with determining the crop coefficient. - Introduced xarray tests to ensure robust data handling and manipulation, improving reliability. - Improved documentation.
VascoSch92
reviewed
May 2, 2024
| pet = ra * (tmean + 5) / 68 / lambd | ||
| ra = extraterrestrial_r(index, lat) | ||
|
|
||
| temp = (tmean + 5) / 68 / lambd |
There was a problem hiding this comment.
Can i suggest to put the parenthesis here to make clear which division is made first.
I'm sure the code is correct but it is not clear which is the first division :)
|
Thank you both for your contributions and for identifying the error! I just returned from holiday and will review the materials as soon as possible. I am proposing the following additions to the Checklist before this PR can be merged:
|
… is nan so instead used rs.values
…is nan so instead used n.values
mvremec
approved these changes
Jun 9, 2024
mvremec
requested changes
Jun 9, 2024
There was a problem hiding this comment.
I mistakenly approved this pull. After a more thorough review, here are some detailed thoughts on the changes:
- Extra Argument: Thank you for catching that!
- Datetime Check: There’s no need to add to_datetime since tmean should already be a DatetimeIndex. To ensure this, we should add a check for tmean.index.
- Oudin Method: The implementation using "pet = pet.where((tmean + k2) >= 0, 0)" is correct as per our model requirements. Let’s keep this as is.
mvremec
reviewed
Jun 9, 2024
| ra = extraterrestrial_r(index, lat) | ||
|
|
||
| temp = (tmean + 5) / 68 / lambd | ||
| temp.index = to_datetime(temp.index) |
There was a problem hiding this comment.
Why use "to_datetime"? The temp should already be DatetimeIndex?
Thank you for your contributions!
|
Hi @anshuverma2000, Thank you for catching the extra argument! I'd appreciate your insights on a couple of other points to help us address the issues effectively:
Best, |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Short Description
Add a short description describing the pull request (PR) here.
Checklist before PR can be merged: