-
Notifications
You must be signed in to change notification settings - Fork 97
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
added two unit tests in test_subsample #312
base: master
Are you sure you want to change the base?
Conversation
Hello @veronewra, thank you for your first contribution to MAPIE. I can help you in this PR. What kind of problem are you experiencing? (In parallel, I've run a few tests on your PR but don't worry, we'll analyze them step by step later.) |
Thank you @thibaultcordier! I get an error when I run make lint (an attribute error from the meta data?), but I can run 'make tests' successfully now! It looks like the "test_split_bootstraps_are_different" unit test I added was failing because I didnt use BlockBootstrap correctly |
mapie/tests/test_subsample.py
Outdated
|
||
def test_split_samples_are_different()->None: | ||
"""Test that subsamples are different """ | ||
X = np.array([0,1,2,3]) |
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 suggest you collect more data to make sure there are no side effects:
At line 8, add this line:
random_state = 42
rng = np.random.default_rng(seed=random_state)
At line 93 (current line):
X = rng.random(100)
mapie/tests/test_subsample.py
Outdated
def test_split_samples_are_different()->None: | ||
"""Test that subsamples are different """ | ||
X = np.array([0,1,2,3]) | ||
cv = Subsample(n_resamplings=2, random_state=1) |
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 suggest you use the previous random_state
:
cv = Subsample(n_resamplings=3, random_state=random_state)
mapie/tests/test_subsample.py
Outdated
with np.testing.assert_raises(AssertionError): | ||
np.testing.assert_equal(trains[0], trains[1]) | ||
np.testing.assert_equal(trains[1], trains[2]) | ||
np.testing.assert_equal(trains[2], trains[3]) |
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.
Attention, the 2nd and 3rd elements don't exist in your example because you've set n_resamplings=2
. I suggest this modification (see the previous comment n_resamplings=3
). Note that the last index is 2
because indexing starts at 0
.
In addition, I suggest this:
with np.testing.assert_raises(AssertionError):
np.testing.assert_equal(trains[0], trains[1])
with np.testing.assert_raises(AssertionError):
np.testing.assert_equal(trains[1], trains[2])
Note that I'm adding a new np.testing.assert_raises
for each test I'd like to run.
mapie/tests/test_subsample.py
Outdated
with np.testing.assert_raises(AssertionError): | ||
np.testing.assert_equal(tests[0], tests[1]) | ||
np.testing.assert_equal(tests[1], tests[2]) | ||
np.testing.assert_equal(tests[2], tests[3]) |
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 can do as previously suggested.
mapie/tests/test_subsample.py
Outdated
def test_split_blockbootstraps_are_different()->None: | ||
"""Test that BlockBootstrap outputs are different """ | ||
X = np.array([0,1,2,3]) | ||
cv = BlockBootstrap(n_blocks=2, n_resamplings=2, random_state=1) |
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 suggest you use the following call:
cv = BlockBootstrap(n_resamplings=3, length=5, random_state=random_state)
mapie/tests/test_subsample.py
Outdated
with np.testing.assert_raises(AssertionError): | ||
np.testing.assert_equal(tests[0], tests[1]) | ||
np.testing.assert_equal(tests[1], tests[2]) | ||
np.testing.assert_equal(tests[2], tests[3]) |
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.
Don't forget to add a new line at the end of the file.
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.
'make lint' complains when I have a blank line at the end of the file:p
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've given you some feedback to solve the problems you've encountered, as well as to make your code more robust. I hope they'll help you pass your tests.
Thank you so much for this feedback! Its definitely helping me understand:) |
Hello @veronewra, do you need support to pursue this PR? Don't hesitate to ask us to help you with any problems you encounter. |
Hello @veronewra, can you tell us whether you plan to continue with this PR? We can discuss together how to follow up this PR. If it's a question of time, we completely understand. In any case, don't hesitate to ask us to help you with any problems you encounter :) |
Thank you so much for your patience and I'm sorry that I abandoned this a
while back due to starting a new job. How about if I can't get this PR in
by Friday, we un-assign the issue from me? I really want to contribute to
MAPIE, but the unfortunate reality is that sometimes I have too many other
priorities:/
All the best,
Veronika
…On Wed, Dec 13, 2023 at 4:46 AM Thibault Cordier ***@***.***> wrote:
Hello @veronewra <https://github.com/veronewra>, do you need support to
pursue this PR? Don't hesitate to ask us to help you with any problems you
encounter.
Hello @veronewra <https://github.com/veronewra>, can you tell us whether
you plan to continue with this PR? We can discuss together how to follow up
this PR. If it's a question of time, we completely understand. In any case,
don't hesitate to ask us to help you with any problems you encounter :)
—
Reply to this email directly, view it on GitHub
<#312 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AL45U26D7Z5QDBYGUQ5QOHDYJGPQVAVCNFSM6AAAAAAZJY3BFOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNJTHA2TIMBTGQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Don't worry, on the contrary, we're always ready to listen to contributors and adapt to their availability. We're reassured to know that you're still motivated to contribute to MAPIE. So don't worry, I'll put this PR on hold and give you time to finish. All the best, |
Description
Hello! I am a first time contributor to MAPIE, and I hope to contribute much more as I get the hang of open source. This pr adds two unit tests in test_subsamples.py that check to make sure that the randomly generated outputs are different. I had some trouble setting up the virtual environment described in the contributing guidelines and would love some help.
Fixes #290
Type of change
-add tests
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist
make lint
make type-check
make tests
make coverage
make doc