JuliaML / LIBLINEAR.jl

LIBLINEAR bindings for Julia
Other
12 stars 10 forks source link

Fix readme example & update to liblinear v247 #26

Closed ericphanson closed 6 months ago

ericphanson commented 11 months ago

With DataFrames v1, data = convert(Matrix, iris[:, 1:4])' doesn't work and we need to use Matrix() instead (these methods were deprecated in v0.22: https://github.com/JuliaData/DataFrames.jl/blob/main/NEWS.md#dataframesjl-v0227-release-notes).

This PR also updates the CI scripts & badge since I don't think travis works anymore, and we can use github actions for windows too, so we don't need appveyor.

This PR also updates to work with liblinear 247 (xref https://github.com/JuliaPackaging/Yggdrasil/pull/7698). I didn't realize the version bump there would be breaking, sorry! I just wanted to build it for macOS m1 so I could run liblinear on my laptop. I believe I have updated the structs here appropriately, comparing to https://github.com/cjlin1/liblinear/compare/v230...v247 (specifically looking linear.h and looking the defaults for the new parameters). All tests pass locally for me.

I have added compat for liblinear so future changes don't break the package, and bumped the Project.toml so this can be registered.

fixes #25

ablaom commented 9 months ago

@innerlee Any chance this could have some attention? See also #27

ablaom commented 8 months ago

@innerlee 🙏🏾

barucden commented 6 months ago

Ok, the tests finished successfully on my machine. Let's merge this!

Thank you, Eric! Great work.

ablaom commented 6 months ago

Okay. Before we tag a new release we need to make a PR to General to change the package URL. I can do this tomorrow.

ablaom commented 6 months ago

@ericphanson @iblislin Any objections to tagging a new release to make this live? Or do we need to wait for #31?

@ericphanson This is a breaking release, right?

barucden commented 6 months ago

31 is just about bumping several github actions to get rid of CI warnings. It has no value or implications for the users.

ericphanson commented 6 months ago

uhh, I think this is not breaking, in that the user-level API (linear_train, linear_predict, etc) has not broken.

What broke workflows was the release of liblinear_jll for v247, due to the combination of 3 things: (1) this package depends on the C struct layouts in liblinear, (2) those changed in the C-library, and (3) this library had no compat bound on liblinear_jll. Users can currently have a working setup if they get the correct version of liblinear_jll (either since they never updated it or they use compat to restrict to the right one).

If we release this PR, what happens is: we declare compat with liblinear_jll, forcing the compatible version. If users update to the new version of LIBLINEAR.jl, they will get updated to the correct corresponding version of liblinear_jll, thanks to compat. If they were successfully using the Julia API (linear_train, linear_predict etc), that code should continue to work. If they were not able to successfully use it due to the wrong liblinear_jll version, this should fix that.

So IMO it is not breaking.

ericphanson commented 6 months ago

No objections to tagging a release, I think that would be good

ablaom commented 6 months ago

Okay, but let me confirm:

ablaom commented 6 months ago

@iblislin I see you already bumped LIBLINEAR.jl to 0.7 on master. Do you think the changes are breaking?

ablaom commented 6 months ago

It seems that dumping Julia 1.3 support alone is not considered breaking.

barucden commented 6 months ago

It was me who bumped that version. Maybe I was too hasty. The recent changes (transfer to JuliaML, this PR, required julia version increased) felt like more than just a patch (0.6.0 -> 0.6.1), so 0.7.0 it is. Given that we are still at major version 0, I did not give it much more thought. I see now that I should have been more careful, so, please, set the version as you see fit.

ablaom commented 6 months ago

The recent changes (transfer to JuliaML, this PR, required julia version increased) felt like more than just a patch (0.6.0 -> 0.6.1), so 0.7.0 it is. Given that we are still at major version 0, I did not give it much more thought.

No that makes sense to me, and I appreciate you taking the initiative here. Let's go with 0.7.

ablaom commented 6 months ago

https://github.com/JuliaRegistries/General/pull/105202