Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

TST add py_loss for tests in _sgd_fast.pyx #18924

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
merged 8 commits into from
Dec 3, 2020

Conversation

TimotheeMathieu
Copy link
Contributor

Reference Issues/PRs

PR arose because of scikit-learn-contrib/scikit-learn-extra#78

What does this implement/fix? Explain your changes.

I add a py_loss function in the class LossFunction in cython file _sgd_fast.pyx.
_sgd_fast.pyx implement a lot of loss functions for regression and classification and allow fast computation of them. I want to use these loss function by simple import but the current implementation only support py_dloss (the derivative of the loss) and not py_loss which returns the loss function.

Any other comments?

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.

Thank you @TimotheeMathieu !

Could you please change

for p, y, expected in cases:
to


def _test_gradient_common(loss_function, cases):
    [...]
    for p, y, expected_loss, expected_dloss in cases:
        assert_almost_equal(loss_function.py_loss(p, y), expected_loss)
        assert_almost_equal(loss_function.py_dloss(p, y), expected_dloss)

and then update the tests that use this helper function in that file to also include the expected value of the loss in the "cases" variable?

To other reviewers: those are normally private objects (no docs) , though imported in linear_models/__init__.py :/ But in any case this would be consistent with what we did for gradient with the py_dloss, and having checks for loss in addition of the gradient wouldn't hurt.

For scikit-learn-extra if would really help if we didn't have to vendor those loss definitions, and could reuse them (knowing that there is no backward compatibility guarantee).

Somewhat related to #15123

@rth rth requested a review from lorentzenchr November 26, 2020 17:14
@rth
Copy link
Member

rth commented Nov 26, 2020

@TimotheeMathieu the only thing is that we don't yet use black in scikit-learn, so the PR should only change the minimal lines needed for this PR. So you would need to revert black related changes in the last commit as unrelated code style changes make using git blame much harder.

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 @TimotheeMathieu !

@glemaitre glemaitre self-requested a review December 2, 2020 14:06
Copy link
Member

@lorentzenchr lorentzenchr left a comment

Choose a reason for hiding this comment

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

@TimotheeMathieu Thanks for this PR. Only one little nitpick from my side.

sklearn/linear_model/tests/test_sgd.py Outdated Show resolved Hide resolved
@lorentzenchr lorentzenchr changed the title add py_loss to _sgd_fast.pyx TST add py_loss for tests in _sgd_fast.pyx Dec 3, 2020
@lorentzenchr lorentzenchr merged commit 7cb6b8f into scikit-learn:master Dec 3, 2020
@cmarmo cmarmo mentioned this pull request Dec 3, 2020
@glemaitre glemaitre mentioned this pull request Dec 22, 2020
14 tasks
@glemaitre glemaitre removed their request for review April 22, 2021 15:52
@glemaitre glemaitre mentioned this pull request Apr 22, 2021
12 tasks
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.

3 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.