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

Attention: Patch coverage is 97.72727% with 2 lines in your changes missing coverage. Please review.

Project coverage is 95.32%. Comparing base (2072e0b) to head (0c5adc0). Report is 18 commits behind head on develop.

Files	Patch %	Lines
pyrealm/core/solar.py	97.33%	2 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## develop #237 +/- ## =========================================== + Coverage 95.24% 95.32% +0.08% =========================================== Files 28 28 Lines 1703 1777 +74 =========================================== + Hits 1622 1694 +72 - Misses 81 83 +2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: 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

[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