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

Fix readme example & update to liblinear v247 #26

Merged
merged 7 commits into from
Apr 17, 2024

Conversation

ericphanson
Copy link
Contributor

@ericphanson ericphanson commented Nov 19, 2023

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 JuliaPackaging/Yggdrasil#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 cjlin1/liblinear@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

@ericphanson ericphanson changed the title Fix readme example Fix readme example & update to liblinear v247 Nov 20, 2023
@ablaom
Copy link
Collaborator

ablaom commented Jan 28, 2024

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

@ablaom
Copy link
Collaborator

ablaom commented Feb 29, 2024

@innerlee 🙏🏾

@barucden
Copy link
Member

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

Thank you, Eric! Great work.

@barucden barucden merged commit 8cc780f into JuliaML:master Apr 17, 2024
1 check passed
@ericphanson ericphanson deleted the eph/fixes branch April 17, 2024 09:12
@ablaom
Copy link
Collaborator

ablaom commented Apr 17, 2024

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
Copy link
Collaborator

ablaom commented Apr 17, 2024

@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
Copy link
Member

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

@ericphanson
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@ablaom
Copy link
Collaborator

ablaom commented Apr 18, 2024

Okay, but let me confirm:

@ablaom
Copy link
Collaborator

ablaom commented Apr 18, 2024

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

@ablaom
Copy link
Collaborator

ablaom commented Apr 18, 2024

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

@barucden
Copy link
Member

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
Copy link
Collaborator

ablaom commented Apr 18, 2024

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
Copy link
Collaborator

ablaom commented Apr 18, 2024

JuliaRegistries/General#105202

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.

Mac M1 support
3 participants