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

Add MLJ usage to the README.md #92

Merged
merged 5 commits into from
Jun 7, 2022
Merged

Add MLJ usage to the README.md #92

merged 5 commits into from
Jun 7, 2022

Conversation

ablaom
Copy link
Contributor

@ablaom ablaom commented Apr 1, 2022

I guess this also closes #90 .

@barucden @till-m

@codecov-commenter
Copy link

codecov-commenter commented Apr 1, 2022

Codecov Report

Merging #92 (8c18b32) into master (56e8d16) will increase coverage by 0.34%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master      #92      +/-   ##
==========================================
+ Coverage   87.63%   87.98%   +0.34%     
==========================================
  Files           6        6              
  Lines         275      283       +8     
==========================================
+ Hits          241      249       +8     
  Misses         34       34              
Impacted Files Coverage Δ
src/nodes.jl 100.00% <0.00%> (ø)
src/LIBSVM.jl 94.11% <0.00%> (+0.03%) ⬆️
src/libcalls.jl 90.00% <0.00%> (+4.28%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 56e8d16...8c18b32. Read the comment docs.

@barucden
Copy link
Member

barucden commented Apr 1, 2022

As far as I can tell, the whole example is written very clearly, and I have no objections in that regard.

However, should this be in the LIBSVM repository? Would it make sense to add this example to the MLJ documentation, and add a section to our README dedicated to higher-level libraries that offer a better interface to LIBSVM?

@till-m
Copy link
Member

till-m commented Apr 1, 2022

I agree with @barucden. Anyone that want's to use LIBSVM.jl via MLJ will probably be drawn to MLJ documentation first and find the necessary information there.

@ablaom I hope you don't feel like @barucden and me reviewing the docstrings in JuliaAI/MLJLIBSVMInterface.jl#13 was somehow conditioned on you doing work for LIBSVM.jl in return. Speaking for myself at least, I can confidently say this is not the case and I am happy to help if I can.

add a section to our README dedicated to higher-level libraries that offer a better interface to LIBSVM?

This seems like a good idea. What other higher-level libraries are there?

E: To be honest, I am still not 100% sure what the problem with #90 is -- based on the error message I suspect that there was something wrong with ytrain?

@ablaom
Copy link
Contributor Author

ablaom commented Apr 3, 2022

Thanks for considering the PR. I don't have a strong opinion about whether it should go ahead. It isn't uncommon for small ML packages to offer a section on different interfaces, but it's extra maintenance.

What if, for now at least, we drop the MLJ example completely but keep the directions for accessing the docstrings? Ie, stop at line 152?

@barucden
Copy link
Member

barucden commented Apr 5, 2022

I would vote for a section dedicated to different interfaces. I just think it's enough to provide a link/reference to these interfaces. I personally only know MLJ so far, but other interfaces may be added later.

However, that's just my opinion. Know that I am not more than an occasional contributor.

@till-m
Copy link
Member

till-m commented Apr 6, 2022

@iblis17 If you have time, I think your input on this discussion would be much appreciated.

I personally only know MLJ so far

PredictMD.jl seems to be another general-purpose framework for ML, but I am not sure it is still being maintained. More specialized packages include e.g. GraphKernels.jl and PosDefManifoldML.jl though I am not sure it would be necessary to list specialized interfaces in any case -- I would think it unlikely someone looking to perform graph-based ML would end up on LIBSVM.jl instead of the actual GraphKernels.jl package GitHub.

@barucden
Copy link
Member

I think it would be great if we could move on with this PR.

@ablaom How would you feel about putting the MLJ-specific guide to perhaps JuliaAI/MLJLIBSVMInterface.jl and putting a link to it to our README?

@ablaom
Copy link
Contributor Author

ablaom commented May 25, 2022

I've reduced the MLJ additions to the docstring references. That's just 10 lines. How's that?

README.md Outdated Show resolved Hide resolved
Co-authored-by: Denis Barucic <barucic.d@gmail.com>
@barucden
Copy link
Member

Thanks! I am in favor of merging.

@till-m
Copy link
Member

till-m commented Jun 7, 2022

@barucden @ablaom how do you want to proceed? There haven't been any complaints, so I would suggest going ahead with the merge sometime soon.

@barucden barucden merged commit c884df6 into JuliaML:master Jun 7, 2022
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.

Code in Readme.md does not work.
4 participants