-
Notifications
You must be signed in to change notification settings - Fork 9
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
Adding two leaf irradience model #245
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great - structure is very compliant with the existing package and popping out the constants class is good. I've added some comments but there isn't any refactoring or major stuff to worry about.
def beta_angle(lat: NDArray, d: NDArray, h: NDArray) -> NDArray: | ||
"""Calculates solar beta angle. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reference for this is page 554 of this:
https://onlinelibrary.wiley.com/doi/epdf/10.1111/j.1365-3040.1997.00094.x
I think beta_angle
here is (or should be) solar elevation angle - which might make it part of the solar functionality rather than an irradiance specific thing. If we move to using one of the existing solar packages, we might get a more accurate model.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added pysolar alternatives in the latest commit which calculate solar elevation directly from latitude, longitude and date time. However, they don't take array inputs natively so you need to use and enumerate operation, so there will be a speed accuracy trade.
The best pysolar solar elevation implementation includes atmospheric distortion, but they also have a faster/simpler version which is the same as the first version I wrote. Because the first version uses numpy vectorisation it may well be faster but I've included the pysolar fast version anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will move these calcs into the core solar library in a sec
lat: array of latitudes (rads) | ||
d: array of declinations (rads) | ||
h: array of hour angle (rads) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've adopted Keith's short variable names in my rough and ready translation to Python, but we should shift to longer names (latitude
, declination
, hour_angle
)?
class TwoLeafIrradience: | ||
"""Calculates two-leaf irradience.""" | ||
|
||
def __init__(self, beta_angle: NDArray, PPFD: NDArray, LAI: NDArray, PATM: NDArray): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although these variable names are acronyms, I think we should stick to lower case variable names: ppfd
, lai
, patm
(and arguably longer - leaf_area_index
, although I know I'm guilty of using patm
and tc
elsewhere.
We could do with a sweep to standardise /check variable names, really!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very good to me, I agree with David's comments. One minor point, I would suggest to start docstrings with capital letters for consistency (and potentially to avoid some warning messages).
@davidorme Could you help me with a query. I have been implementing the irradience model into a Pyrealm irradience class, and along the way separating out the individual functions. It appears that a variable defined in the .md is surplus to requirements as it is never used. I have checked dePury& Farquhar(1997) and this appears to be the case. The extraneous variable is |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #245 +/- ##
===========================================
- Coverage 95.24% 88.57% -6.68%
===========================================
Files 28 30 +2
Lines 1703 1899 +196
===========================================
+ Hits 1622 1682 +60
- Misses 81 217 +136 ☔ View full report in Codecov by Sentry. |
updates: - [github.com/igorshubovych/markdownlint-cli: v0.40.0 → v0.41.0](igorshubovych/markdownlint-cli@v0.40.0...v0.41.0)
…eLondon/pyrealm into Two_leaf_irradience_model
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Description
Adding irradience calculation class, supporting functions and constants dataclass for the two leaf canopy model implementation.
Fixes #236 and #244
Type of change
Key checklist
pre-commit
checks:$ pre-commit run -a
$ poetry run pytest
Further checks