Add two `SIRUS.jl` models

rikhuijzer commented 1 year ago

It took a bit longer than expected, but finally regression was implemented for SIRUS.jl. I've tried to explain at the Julia Discourse why this is a useful model. Hopefully the StableRulesClassifier and StableRulesRegressor can be added to MLJModels.jl :smile:

codecov-commenter commented 1 year ago

Codecov Report

Merging #519 (7776dac) into dev (c72e67c) will decrease coverage by 0.16%. The diff coverage is n/a.

:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##              dev     #519      +/-   ##
==========================================
- Coverage   76.92%   76.76%   -0.16%     
==========================================
  Files          16       16              
  Lines        1157     1162       +5     
==========================================
+ Hits          890      892       +2     
- Misses        267      270       +3

see 2 files with indirect coverage changes

ablaom commented 1 year ago

Congratulations! Looks like a very thorough piece of work.

I'm currently on leave so copying @OkonSamuel in here. I strongly suggest you take a look at:

[x] https://github.com/rikhuijzer/SIRUS.jl/issues/29

OkonSamuel commented 1 year ago

Thanks @rikhuijzer I'll review this weekend.

ablaom commented 1 year ago

See also:

[ ] https://github.com/rikhuijzer/SIRUS.jl/issues/32.

OkonSamuel commented 1 year ago

Thanks @rikhuijzer. This is indeed a great addition. Your code is clean :smile: Just adjust the docstrings a bit to match the mlj standard.

rikhuijzer commented 12 months ago

I hope this is now conforming the spec. I also refactored the code a bit in https://github.com/rikhuijzer/SIRUS.jl/pull/37 and things are definitely more clear now :+1:

If this PR is accepted, then I'll register the new SIRUS.jl version with the updated docstrings (v1.2.2).

OkonSamuel commented 11 months ago

I hope this is now conforming the spec. I also refactored the code a bit in rikhuijzer/SIRUS.jl#37 and things are definitely more clear now 👍

If this PR is accepted, then I'll register the new SIRUS.jl version with the updated docstrings (v1.2.2).

@rikhuijzer Thanks for addressing most of the issues raised. I'm sorry for being so meticulous here but there are just a few more things that needed to be added/removed before merging.

[x] Avoid using docstring keyword in the metadata_model helper. I have opened a PR here to handle that.
[x] Add Operations and Fitted parameters section to the docstring. You can use https://github.com/JuliaAI/NearestNeighborModels.jl/blob/dev/src/models.jl#L575-L583 as template for Classification and https://github.com/JuliaAI/NearestNeighborModels.jl/blob/dev/src/models.jl#L731-L736 as a template for Regression. Feel free to adjust the descriptions to match your model's context. Since you didn't define a fitted_params specific to your model types, mlj calls the default implementation.
[x] Although Optional, it is highly recommended you add an Examples section. You can use the one you defined here

I'm happy to help out in a way I can.