Closed ablaom closed 2 years ago
Merging #92 (8c18b32) into master (56e8d16) will increase coverage by
0.34%
. The diff coverage isn/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%) |
:arrow_up: |
src/libcalls.jl | 90.00% <0.00%> (+4.28%) |
:arrow_up: |
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.
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?
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
?
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?
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.
@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.
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?
I've reduced the MLJ additions to the docstring references. That's just 10 lines. How's that?
Thanks! I am in favor of merging.
@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.
I guess this also closes #90 .
@barucden @till-m