236 develop pyrealmcoresolar function radiation calc library

[j-emberton]

Description

Refactored pyreal.splash.solar to move core calculations to pyrealm.core.solar.
Functionality of Splash module is unchanged
Change is to facilitate development of two-leaf canopy model

Fixes #236

Type of change

• New feature (non-breaking change which adds functionality)
• Optimization (back-end change that speeds up the code)
• Bug fix (non-breaking change which fixes an issue)

Key checklist

• Make sure you've run the pre-commit checks: $ pre-commit run -a
• All tests pass: $ poetry run pytest

Further checks

• Code is commented, particularly in hard-to-understand areas
• Tests added that prove fix is effective or that feature works

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.34%. Comparing base (1f315ba) to head (486e91a).
Report is 342 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #237      +/-   ##
===========================================
+ Coverage    95.29%   95.34%   +0.05%     
===========================================
  Files           28       35       +7     
  Lines         1720     2751    +1031     
===========================================
+ Hits          1639     2623     +984     
- Misses          81      128      +47     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[j-emberton]

@davidorme would it be possible for you to have a quick look over my core.solar functions and the unit tests to make sure they're in line with your code standards. At the moment I've just tested basic maths using representative values as there's no error handling in the functions. I have a spreadsheet with the calcs implemented so it's trivial to add any further tests should they be required. If you would like any basic checks added such as array length equality, just let me know, but for now I presumed data sanitisation would have happened upstream of these calcs.

[davidorme]

@j-emberton That all looks good. In the past, I wouldn't have tended to use pytest.parameterise when there is only one parameter set, but this makes it easier to extend and better isolates/identifies the inputs, so is probably better practice.

One awkward thing in the API are the functions calc_sunset_hour_angle and calc_daily_solar_radiation taking those intermediate variables ru and rv rather than directly accepting delta and lat. There's good reason for that in splash usage - ru and rv are used repeatedly in different calculations so calculating them once is more efficient, but if someone is using them more directly, then it is a bit obscure. Having said that, the API does get a bit clumsy if you either provide delta and lat (and create ru and rv internally) or provide ru and rv...

[davidorme]

Oh - wait... Will post a suggestion.

[j-emberton]

@davidorme this library seems to be working now and (at least locally) all tests pass.

I incorporated your proposed change to calculate ru and rv from within calc_hour_angle. However, if we apply this throughout splash.solar, then ru and rv get calculated multiple times instead of just once adding computational cost.

If we compile these functions with cython or number it probably doesn't matter too much unless the arrays are huge.

What would you prefer?

[davidorme]

I guess I was thinking that we extend that same structure to the other functions that use ru and rv. So individual users of the stand alone functions might provide delta and latitude, but all of those functions are backed by a short cut method that accepts ru and rv.

I'm not sure how complex that looks - I think my approach here is that users see functions with straightforward inputs and don't have to manual calculate those intermediate inputs, but those functions can then wrap the calculation of the intermediate values and an internal calculation, which we can use more directly inside routines.

That does feel a bit around the houses, I admit.

[j-emberton]

One way around it would be to put these library functions inside a class and then have self.ru and self.rv generated upon creating a class instance.

public methods would then have access to these variables by default.

What I'll do for now is go ahead and implement as per your suggestion. We can then profile the code and see if this is an issue or not. If it is, we have a good place to refactor for performance improvements as we have tests ready to go.

[j-emberton]

One point I haven't put lots of thought into is whether we want more extensive documentation updates now we've added more functionality to the core.solar module

[vgro]

LGTM

[j-emberton]

@davidorme

If I add some user guidance on how to access the functions in this library, where should I put it. Do we want it to go under core utilities, or create a new solar utilities section?

[j-emberton]

moving back to draft while I add a few more bits

[davidorme]

If I add some user guidance on how to access the functions in this library, where should I put it. Do we want it to go under core utilities, or create a new solar utilities section?

Definitely a new core.solar section.

[j-emberton]

On top of what was in there before I've added:
new functions:

• calc_ppfd - wrapper to calculate pfd from elementary inputs
• calc_declination - function to calculate solar declination
• calc_beta_angle_from_lat_dec_hour - function to calculate solar elevation

Tests for the above functions

Added detail like examples and latex maths to some of the docstrings to function as public facing documentation (where needed)

[j-emberton]

Once we get a couple more papers into the bibliography we can add proper citations for the implemented maths

[davidorme]

This looks great. Only minor things:

• Some docstring formatting issues. I've pushed a commit to include the solar docs in the API section, so we can review how they render. I've made some general suggestions on one of the docstrings that apply across the module.
• Using named arguments - I'm not brilliant at practicing this, but with long lists of args, it's much more robust.
• I think the helper wrapper functions could take a packaged CoreConst object to make them more usable.

[j-emberton]

Thankyou for the feedback @davidorme. Much appreciated. I thought we had agreed to make all the variables explicit but I may have got my wires crossed. In any case I'm happy to swap in the core const and adjust as required. I'll do it for all the library functions I create for consistency. It makes sense since those objects will usually be available a priori and don't require any bespoke setup.

[davidorme]

I don't think you got your wires crossed - it's a grey area and we don't have a consistent approach. It's difficult to come up with a hard and fast rule and we're not systematic about this, but I think it's something like "If a function is passing constants on to lower level functions, then passing in constants classes is probably easier for the user and more compact."

Changes made

[j-emberton]

OK, I think this is all working now and docs basically look ok, although I appreciate there maybe some style tweaks required - lets do these in a followup issue if that's ok. I'd like to just get this work in the bank.

[davidorme]

I agree that we should bank this functionality and worry about documentation polishing later. The docs are readable and useable now.

One oddity noted in a comment - there are a lot of line endings marked with \. Those don't appear in the built docs, so aren't a problem, but also aren't required and aren't used elsewhere in the project.