added two unit tests in test_subsample #312

veronewra · 2023-06-16T22:07:29Z

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

Test A
Test B

Checklist

I have read the contributing guidelines
I have updated the HISTORY.rst and AUTHORS.rst files
Linting passes successfully : make lint
Typing passes successfully : make type-check
Unit tests pass successfully : make tests
Coverage is 100% : make coverage
Documentation builds successfully : make doc

thibaultcordier · 2023-06-19T10:00:11Z

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.)

…tTests

veronewra · 2023-06-23T19:55:34Z

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

thibaultcordier · 2023-06-27T08:33:16Z

mapie/tests/test_subsample.py

+
+def test_split_samples_are_different()->None:
+    """Test that subsamples are different """
+    X = np.array([0,1,2,3])


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)

thibaultcordier · 2023-06-27T08:37:38Z

mapie/tests/test_subsample.py

+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)


I suggest you use the previous random_state:

cv = Subsample(n_resamplings=3, random_state=random_state)

thibaultcordier · 2023-06-27T08:41:25Z

mapie/tests/test_subsample.py

+    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])


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.

thibaultcordier · 2023-06-27T08:42:07Z

mapie/tests/test_subsample.py

+    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])


You can do as previously suggested.

mapie/tests/test_subsample.py

thibaultcordier · 2023-06-27T08:48:02Z

mapie/tests/test_subsample.py

+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)


I suggest you use the following call:

cv = BlockBootstrap(n_resamplings=3, length=5, random_state=random_state)

thibaultcordier · 2023-06-27T08:48:47Z

mapie/tests/test_subsample.py

+    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])  


Don't forget to add a new line at the end of the file.

'make lint' complains when I have a blank line at the end of the file:p

thibaultcordier

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.

veronewra · 2023-06-29T00:55:32Z

Thank you so much for this feedback! Its definitely helping me understand:)

thibaultcordier · 2023-10-19T09:36:26Z

Hello @veronewra, do you need support to pursue this PR? Don't hesitate to ask us to help you with any problems you encounter.

thibaultcordier · 2023-12-13T12:45:51Z

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 :)

veronewra · 2023-12-13T20:15:20Z

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: ***@***.***>

thibaultcordier · 2023-12-14T17:21:07Z

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

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,

added two unit tests in test_subsample

d6bdeaa

thibaultcordier self-requested a review June 19, 2023 09:51

thibaultcordier assigned veronewra Jun 19, 2023

veronewra and others added 3 commits June 23, 2023 11:59

unit tests should pass now

a7e89c6

Merge branch 'scikit-learn-contrib:master' into addUnitTests

5707313

Merge branch 'addUnitTests' of github.com:veronewra/MAPIE into addUni…

23830b7

…tTests

thibaultcordier reviewed Jun 27, 2023

View reviewed changes

mapie/tests/test_subsample.py Show resolved Hide resolved

thibaultcordier reviewed Jun 27, 2023

View reviewed changes

thibaultcordier requested changes Jun 27, 2023

View reviewed changes

long overdue- incorperating thibaultcordiers suggestions

c993d8f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added two unit tests in test_subsample #312

added two unit tests in test_subsample #312

veronewra commented Jun 16, 2023 •

edited by LacombeLouis

thibaultcordier commented Jun 19, 2023 •

edited

veronewra commented Jun 23, 2023

thibaultcordier Jun 27, 2023 •

edited

thibaultcordier Jun 27, 2023 •

edited

thibaultcordier Jun 27, 2023 •

edited

thibaultcordier Jun 27, 2023

thibaultcordier Jun 27, 2023 •

edited

thibaultcordier Jun 27, 2023

veronewra Dec 14, 2023

thibaultcordier left a comment

veronewra commented Jun 29, 2023

thibaultcordier commented Oct 19, 2023

thibaultcordier commented Dec 13, 2023

veronewra commented Dec 13, 2023 via email

thibaultcordier commented Dec 14, 2023

added two unit tests in test_subsample #312

Are you sure you want to change the base?

added two unit tests in test_subsample #312

Conversation

veronewra commented Jun 16, 2023 • edited by LacombeLouis

Description

Type of change

How Has This Been Tested?

Checklist

thibaultcordier commented Jun 19, 2023 • edited

veronewra commented Jun 23, 2023

thibaultcordier Jun 27, 2023 • edited

Choose a reason for hiding this comment

thibaultcordier Jun 27, 2023 • edited

Choose a reason for hiding this comment

thibaultcordier Jun 27, 2023 • edited

Choose a reason for hiding this comment

thibaultcordier Jun 27, 2023

Choose a reason for hiding this comment

thibaultcordier Jun 27, 2023 • edited

Choose a reason for hiding this comment

thibaultcordier Jun 27, 2023

Choose a reason for hiding this comment

veronewra Dec 14, 2023

Choose a reason for hiding this comment

thibaultcordier left a comment

Choose a reason for hiding this comment

veronewra commented Jun 29, 2023

thibaultcordier commented Oct 19, 2023

thibaultcordier commented Dec 13, 2023

veronewra commented Dec 13, 2023 via email

thibaultcordier commented Dec 14, 2023

veronewra commented Jun 16, 2023 •

edited by LacombeLouis

thibaultcordier commented Jun 19, 2023 •

edited

thibaultcordier Jun 27, 2023 •

edited

thibaultcordier Jun 27, 2023 •

edited

thibaultcordier Jun 27, 2023 •

edited

thibaultcordier Jun 27, 2023 •

edited