Skip to content
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

Create MLJ compliant doc strings #138

Merged
merged 8 commits into from
Feb 1, 2023
Merged

Create MLJ compliant doc strings #138

merged 8 commits into from
Feb 1, 2023

Conversation

ablaom
Copy link
Member

@ablaom ablaom commented Jan 27, 2023

@tlienart

Note to self: We should probably add MLJTestInterface tests before tagging next release.

@ablaom ablaom marked this pull request as draft January 27, 2023 03:22
@ablaom ablaom changed the title Creat MLJ compliant doc strings Create MLJ compliant doc strings Jan 27, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jan 27, 2023

Codecov Report

Merging #138 (7c9e435) into dev (30f7a30) will increase coverage by 0.65%.
The diff coverage is 0.00%.

❗ Current head 7c9e435 differs from pull request most recent head bd0a20f. Consider uploading reports for the commit bd0a20f to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##              dev     #138      +/-   ##
==========================================
+ Coverage   94.62%   95.28%   +0.65%     
==========================================
  Files          20       21       +1     
  Lines         875      869       -6     
==========================================
  Hits          828      828              
+ Misses         47       41       -6     
Impacted Files Coverage Δ
src/mlj/classifiers.jl 100.00% <ø> (+50.00%) ⬆️
src/mlj/doc_tools.jl 0.00% <0.00%> (ø)
src/mlj/interface.jl 85.45% <0.00%> (+4.42%) ⬆️
src/mlj/regressors.jl 12.50% <ø> (+5.83%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

src/mlj/classifiers.jl Outdated Show resolved Hide resolved
$(doc_header(MultinomialClassifier))

This model coincides with [`LogisticClassifier`](@ref), except certain optimizations
possible in the special binary case will not be applied. It's hyperparameters are
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Its

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. Addressed.

src/mlj/classifiers.jl Outdated Show resolved Hide resolved
@ablaom ablaom marked this pull request as ready for review February 1, 2023 03:20
@ablaom
Copy link
Member Author

ablaom commented Feb 1, 2023

Thanks for all the help, @tlienart. This PR is now ready for review. Please pay particular attention to the solver field documentation.

One remaining point of confusion for me is this: I'm interpreting the existing docs and what your have helped with as "ISTA and FISTA are disallowed if gamma = 0" but I think you only mean to say these are the only options when gamma > 0 (because we need a non-smooth solver in that case) - and not that it can't still be used when gamma = 0 as well (although better options exist in that case). So, in this PR, I've said (F)ISTA solvers are allowed for any model.

@tlienart
Copy link
Collaborator

tlienart commented Feb 1, 2023

One remaining point of confusion for me is this: I'm interpreting the existing docs and what your have helped with as "ISTA and FISTA are disallowed if gamma = 0" but I think you only mean to say these are the only options when gamma > 0 (because we need a non-smooth solver in that case) - and not that it can't still be used when gamma = 0 as well (although better options exist in that case). So, in this PR, I've said (F)ISTA solvers are allowed for any model.

This is correct, with gamma=0 the prox operation will be the identity and it will be a kind of gradient descent which should generally be ok (I've not tested but I don't see a reason why it would fail; I'll open an issue to test this explicitly)

Copy link
Collaborator

@tlienart tlienart left a comment

Choose a reason for hiding this comment

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

LGTM with minor changes; feel free to merge after that

src/mlj/classifiers.jl Outdated Show resolved Hide resolved
src/mlj/classifiers.jl Show resolved Hide resolved
src/mlj/classifiers.jl Outdated Show resolved Hide resolved
src/mlj/classifiers.jl Outdated Show resolved Hide resolved
src/mlj/doc_tools.jl Show resolved Hide resolved
@tlienart tlienart merged commit 9d98288 into dev Feb 1, 2023
@tlienart
Copy link
Collaborator

tlienart commented Feb 1, 2023

I imagine you might want to do a patch release with this, let me know if you need my help in doing so (I think you have full admin access to this repo though). Thanks for going through this @ablaom !

@ablaom
Copy link
Member Author

ablaom commented Feb 1, 2023

Just going to check #140 to be sure.

@ablaom ablaom mentioned this pull request Feb 1, 2023
@ablaom
Copy link
Member Author

ablaom commented Feb 1, 2023

Before the patch we should address #141

@ablaom ablaom mentioned this pull request Feb 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants