Add effective radius output#311
Conversation
thabbott
left a comment
thabbott
left a comment
There was a problem hiding this comment.
This looks good overall! A lot of changes have been merged into main since you opened this PR, so before you do anything else you should rebase your branch onto the contrailcirrus main branch:
git remote add upstream https://github.com/contrailcirrus/pycontrails
git fetch upstream
git rebase upstream/mainIf this is successful that latest changes in CHANGELOG.md should be for v0.54.11.
After that you should make sure that tests pass locally. To do that, run
GITHUB_ACTIONS=true make test
(Setting GITHUB_ACTIONS=true disables some slow LEO datalib tests that don't matter for this PR.) If the tests fail, try re-installing your pycontrails environment to make sure your dependencies are all up to date.
Once you confirm tests pass, push the changes to update this PR (you'll have to force push after rebasing).
A few other things, in addition to those in other comments:
- Can you add a description of your changes to
CHANGELOG.md? - I don't think any of the pinned test results in static json files should need updating... can you try re-running tests without those changes and see if they still pass?
- Can you add a similar set of tests for gridded CoCip?
| verbose_outputs: bool = False | ||
|
|
||
| #: Add effective radius to contrail. | ||
| #: It is not necessary for calculation. |
There was a problem hiding this comment.
| #: It is not necessary for calculation. |
| #: :attr:`CocipGridParams.verbose_outputs_evolution`. | ||
| verbose_outputs: bool = False | ||
|
|
||
| #: Add effective radius to contrail. |
There was a problem hiding this comment.
| #: Add effective radius to contrail. | |
| #: Add effective radius to contrail output. |
| radius_threshold_um: npt.NDArray[np.floating], | ||
| ) -> npt.NDArray[np.floating]: | ||
| r""" | ||
| Calculate the effective radius of contrail ice particles for each waypoints. |
There was a problem hiding this comment.
| Calculate the effective radius of contrail ice particles for each waypoints. | |
| Calculate the effective radius of contrail ice particles for each waypoint. |
| r""" | ||
| Calculate the effective radius of contrail ice particles for each waypoints. | ||
|
|
||
| For each waypoints, all the habits are assumed by applying a weighted harmonic mean. |
There was a problem hiding this comment.
Can you add a brief explanation of why a harmonic mean is required?
| assert 0 < ratio < 1 | ||
|
|
||
|
|
||
| def test_effective_radius(cocip_persistent: Cocip): |
There was a problem hiding this comment.
This doesn't actually test whether effective radius gets attached to CoCiP output when params["output_effective_radius"] is True. Rather than calling effective_radius directly in the test, I'd create a new fixture with effective radius output enabled and test the effective radius values in that fixture.
…roperties (to allow to have the effective radius in CoCiPGrid too) only if output_effective_radius is True
…_effective_radius = False. Verify also the reff value.
… doesn't work for the moment, even if the output_effective_radius parameter is True, the reff parameter isn't in the output when running CocipGrid.
nathgrimmer
force-pushed
the
feature/reff-output
branch
from
3e42f05 to
598b0c1
Compare
2 participants
Closes #XX
Changes
Breaking changes
Features
Fixes
Internals
Tests
make test)Reviewer