-
Notifications
You must be signed in to change notification settings - Fork 3
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
Input check #81
Input check #81
Conversation
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.
Looks good.
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.
The changes look good to me. Let me know if I can suggest on nuisances.py with the changes that I can state here again:
delete lines 122-125
if len(x.shape) == 1:
x = x.reshape(-1, 1)
if len(m.shape) == 1:
m = m.reshape(-1, 1)
delete line 167-168
if len(x.shape) == 1:
x = x.reshape(-1, 1)
- replace line 231-236:
if len(x.shape) == 1:
x = x.reshape(-1, 1)
if len(m.shape) == 1:
mr = m.reshape(-1, 1)
else:
mr = np.copy(m)
by mr = np.copy(m)
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.
The PR looks good, but probably needs more tests.
So tests pass, however I think they should not. I missed this modification of tests, 4f63b80 but it results on the exactness tests being no super useful as they do not catch modifications of the behavior of the estimators. And the threshold of |
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## develop #81 +/- ##
===========================================
+ Coverage 86.53% 89.08% +2.55%
===========================================
Files 6 6
Lines 750 733 -17
===========================================
+ Hits 649 653 +4
+ Misses 101 80 -21 ☔ View full report in Codecov by Sentry. |
indeed. I think it would be nice to test each case where the check_input function raises + the output format. I don't know if you have other tests in mind @bthirion ? As discussed @brash6 we can work on it together depending on who has time first |
Sorry, I would inspect uncovered lines of code. |
The changes LGTM. But we need a green CI... |
I have modified the tests to make them simpler and to split them (instead of testing several things in the same test). Not sure which one is best. Let me know, but otherwise, I think it's ok, and it will allow to merge the fixes of the CI. |
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.
LGTM, besides one small detail.
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.
If you can do a pass to have consistent argument order, that would be great.
if n * dim_m != sum(m.ravel() == 1) + sum(m.ravel() == 0): | ||
raise ValueError("m is not binary") | ||
# check input | ||
y, t, m, x = _check_input(y, t, m, x, setting='binary') |
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.
Please make sure that the argument order y, t, m, x
is there in all functions. Sometimes, there is t, m, x, y
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.
LGTM. Will push 2 minor things
med_bench/src/tests/estimation/test_get_estimation.py
Line 257 in 4ebee9c
still a draft (I know there are commented lines to remove or this sort of things), I have issues with
I open the PR to discuss if the main direction is ok for everyone (sorry, in develop, not main)