-
Notifications
You must be signed in to change notification settings - Fork 18
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
Add tests for Power Transformer #430
Add tests for Power Transformer #430
Conversation
Hm why are we not getting a codecov report here? |
The log gives a hint: https://github.com/MESMER-group/mesmer/pull/430/checks#step:8:55 Can't say that I understand it, though 🤭. I thought codecov was moving away from tokens, but they seem to recommend it again. Let me check what xarray does... Also why only this PR and why consistently? |
Ah I think it's here: https://docs.codecov.com/docs/codecov-uploader#supporting-token-less-uploads-for-forks-of-open-source-repos-using-codecov (the last paragraph) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a look from my smartphone...
for more information, see https://pre-commit.ci
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #430 +/- ##
==========================================
- Coverage 87.90% 86.79% -1.12%
==========================================
Files 40 44 +4
Lines 1745 1954 +209
==========================================
+ Hits 1534 1696 +162
- Misses 211 258 +47
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Ah now also the Codecov report's here :) |
We could add another test, but it is not really code related but rather of scientific nature: transform skewed data and test if it is actually less skewed afterwards. What do you think @mathause ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I add some more suggestions after I understand it a bit better. However, this whole thing is a quite a mess (not your PR - the thing overall). I'll need another pass once we cleaned this up some more and we start to see through.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You ignored some two of my comments, which I think are still valid.
for more information, see https://pre-commit.ci
Sorry, hadn't pushed them yet. |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Okay this got messy but now it's ready to merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Thanks.
No description provided.