Skip to content
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

Remove power transformer class #474

Merged

Conversation

veni-vidi-vici-dormivi
Copy link
Collaborator

In this PR I am removing the power transformer class in favor of the newly written xarray functions and adjusting the tests accordingly.

@veni-vidi-vici-dormivi
Copy link
Collaborator Author

I now deleted the PowerTransformerVariableLambda class. Should we instead deprecate it properly? I thought it is unnecessary because because it was never properly implemented. What do you think @mathause?

Copy link

codecov bot commented Jun 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 51.53%. Comparing base (9b0b76b) to head (4745f11).
Report is 104 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main     #474       +/-   ##
===========================================
- Coverage   87.90%   51.53%   -36.38%     
===========================================
  Files          40       50       +10     
  Lines        1745     3252     +1507     
===========================================
+ Hits         1534     1676      +142     
- Misses        211     1576     +1365     
Flag Coverage Δ
unittests 51.53% <ø> (-36.38%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Copy link
Member

@mathause mathause left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good for me to just remove it. It also disentangles it as the PoerTransformer can also do a box-cox transformation which was however not adapted for variable $\lambda$.

@veni-vidi-vici-dormivi
Copy link
Collaborator Author

I think the only point where we still need sklearn now is the tests. So do I see that correctly that we could now (again) have a separate environment for testing and for the users and exclude sklearn in the users environment, or not? Even if so, I would keep it at one environment at this point I think.

@mathause
Copy link
Member

mathause commented Jul 2, 2024

It's always nice to have less dependencies but we can keep it for the moment. Maybe add a comment in setup.cfg?

@veni-vidi-vici-dormivi veni-vidi-vici-dormivi merged commit 526f919 into MESMER-group:main Jul 2, 2024
9 checks passed
@veni-vidi-vici-dormivi veni-vidi-vici-dormivi deleted the ptrmclass branch July 2, 2024 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test PowerTransformer against sklearn (for constant lambda)
2 participants