Skip to content

TST replace assert_raise_* by pytest.raises in linear_model module #19399 #19440

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

Merged

Conversation

YasmeenAlsaedy
Copy link
Contributor

Reference Issues/PRs

This PR is related to issue #14216

What does this implement/fix? Explain your changes.

Change assert_raises and assert_raises_regex to :
with pytest.raises(.....)

This is a part of Data Umbrella's sprint that was held in February 2021.

…ests/test_coordinate_descent.py, test_logistic.py, test_passive_aggressive.py, test_perceptron.py, test_ransac.py, test_ridge.py, test_sag.py, test_sgd.py
…ests/test_coordinate_descent.py, test_logistic.py, test_passive_aggressive.py, test_perceptron.py, test_ransac.py, test_ridge.py, test_sag.py, test_sgd.py: Fixed the test issues
@YasmeenAlsaedy YasmeenAlsaedy force-pushed the update_linearmodelRemoveAssertRaises branch from 1e33453 to a62f1a4 Compare February 12, 2021 09:08
@YasmeenAlsaedy YasmeenAlsaedy changed the title TST replace assert_raise_* by pytest.raises in ensemble module #19399 TST replace assert_raise_* by pytest.raises in liner model module #19399 Feb 12, 2021
@YasmeenAlsaedy YasmeenAlsaedy changed the title TST replace assert_raise_* by pytest.raises in liner model module #19399 TST replace assert_raise_* by pytest.raises in linear_model module #19399 Feb 12, 2021
@glemaitre glemaitre self-requested a review February 12, 2021 13:26
Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM

@YasmeenAlsaedy
Copy link
Contributor Author

Thank you @glemaitre for all your help :).

Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

You still have issue with the linter and some conflicts.

@YasmeenAlsaedy
Copy link
Contributor Author

I removed the Conflict and marked it as ( Resolved )

Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

Thanks! A few minor comments otherwise LGTM.

Further details can be found in the appendix of [2].

Copy link
Member

Choose a reason for hiding this comment

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

Also the above empty lines are intentional and shouldn't be removed.

Copy link
Member

Choose a reason for hiding this comment

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

and below.

@YasmeenAlsaedy YasmeenAlsaedy requested a review from rth February 12, 2021 18:30
Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

+1 also on my side once the remaining comment by @rth has been addressed:

#19440 (comment)

Further details can be found in the appendix of [2].

Reference
---------
.. [1] Zhu, Ji and Trevor J. Hastie. "Classification of gene microarrays by
penalized logistic regression". Biostatistics 5 3 (2004): 427-43.
https://doi.org/10.1093/biostatistics%2Fkxg046

Copy link
Member

Choose a reason for hiding this comment

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

This line should be kept (I think). We are not that consistent in scikit-learn but I think more often than not, we have blank lines between references.

@YasmeenAlsaedy
Copy link
Contributor Author

Thank you all for your help ^_^

@rth rth merged commit dac5605 into scikit-learn:main Feb 12, 2021
@reshamas
Copy link
Member

#DataUmbrella
cc: @Mariam-ke

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants