MESMER-group / mesmer

spatially-resolved ESM-specific multi-scenario initial-condition ensemble emulator
https://mesmer-emulator.readthedocs.io/en/latest/
GNU General Public License v3.0
23 stars 17 forks source link

Write Power Transformer functions for xarray compatibility #442

Closed veni-vidi-vici-dormivi closed 3 months ago

veni-vidi-vici-dormivi commented 4 months ago

Here I write functions for the power transformer to take in xarrays.

codecov[bot] commented 4 months ago

Codecov Report

Attention: Patch coverage is 87.80488% with 10 lines in your changes missing coverage. Please review.

Project coverage is 52.53%. Comparing base (9b0b76b) to head (79a21b9). Report is 102 commits behind head on main.

Files with missing lines Patch % Lines
mesmer/mesmer_m/power_transformer.py 87.80% 10 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #442 +/- ## =========================================== - Coverage 87.90% 52.53% -35.38% =========================================== Files 40 50 +10 Lines 1745 3320 +1575 =========================================== + Hits 1534 1744 +210 - Misses 211 1576 +1365 ``` | [Flag](https://app.codecov.io/gh/MESMER-group/mesmer/pull/442/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MESMER-group) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/MESMER-group/mesmer/pull/442/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MESMER-group) | `52.53% <87.80%> (-35.38%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MESMER-group#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

veni-vidi-vici-dormivi commented 4 months ago

At the moment I am appending functions for the xarray power transformer below the old functions. I also now opted to write functions instead of class methods. This is WIP though we can build on this first draft here.

veni-vidi-vici-dormivi commented 3 months ago

Can you fix the docstrings and comments so they no wider than the 88 characters.

Does the precommit hook not work for docstrings and comments?

mathause commented 3 months ago

Can you fix the docstrings and comments so they no wider than the 88 characters.

Does the precommit hook not work for docstrings and comments?

Ah no unfortunately not - could be worthwhile to check if there is a tool (although automatic string manipulation is a tricky business...)

veni-vidi-vici-dormivi commented 3 months ago

What is your suggestion for the next steps

I wrote this with the goal of removing the class since I felt it is weird to have a class for the power transformer but not for the harmonic model. On the other hand for the harmonic model it's just one function call and for the power transformer it's three.

Looking at MESMER we have the LinearRegression Class which also works well. I am not surre though how elegant I find it to save the parameters needed for the Powertransformer as a netcdf and then having to assign the parameters to the class for the emulation as for the LinearRegression...

What do you think?

mathause commented 3 months ago

Yes I am no longer happy about the approach I took for the linear regression - especially the lr.params = params part. Is creating a class just a cheap way around the flatter hierarchy we discussed in #348 (i.e. having everything in the mesmer.stats namespace)? But then again - having 3-4 functions for each method makes it cluttered fast... Really not sure what is best... we may have to re-visit #348 at some point...

Sticking with functions for now, maybe?

mesmer.stats.yeo_johnson_fit(...)
mesmer.stats.yeo_johnson_transform(...)
mesmer.stats.yeo_johnson_inverse_transform(...)