search

rust-ml / linfa

A Rust machine learning framework.
Apache License 2.0
3.7k stars 243 forks source link

Bump `argmin` dependency to 0.8 #303

Closed YuhanLiin closed 1 year ago

YuhanLiin commented 1 year ago

Now that our MSRV is higher than 1.62, we can use argmin v0.8. This update will also allow linfa-ftrl and linfa-logistic to have serde as an optional feature.

IbrahimBagalwa commented 1 year ago

Hello @YuhanLiin I'm interested in contributing to this open source project. However, I'm not entirely clear on what this issue entails and would greatly appreciate some clarification.

Could you please provide me with more information about the issue and what specifically needs to be done to resolve it? I'm eager to learn and contribute in any way I can. thank you

YuhanLiin commented 1 year ago

So linfa-ftrl and linfa-logistic both depend on argmin 0.4.6 and linfa-linear has argmin 0.7. I'd like the argmin version on all three crates to be bumped up to 0.8, and argmin-math should be bumped to 0.3. The change should be trivial on linfa-linear, but will be more involved on linfa-logistic and linfa-ctrl, since they require code changes. You can refer to #289, which bumped argmin in linfa-linear to 0.7, for guidance on what to change in the other two crates.

The reason why I want to upgrade argmin is because newer versions of argmin have serde as an optional dependency, whereas serde was mandatory in older versions. This in turn allows linfa-ftrl and linfa-logistic to have serde as optional, which is consistent with the other Linfa crates. Again, you can refer to #289, which also made serde optional in linfa-linear.

stefan-k commented 1 year ago

I planned to update linfa-ftrl and linfa-logistic as well but unfortunately I won't be able get to it anytime soon. I may however be able to provide guidance and help if needed. Feel free to ping me if there is anything I may be able to help you with.

If I recall correctly, the changes necessary in linfa-ftrl should be fairly similar to the ones made in linfa-linear. I think linfa-logistic may be a bit more complicated.

JadenMajid commented 1 year ago

Hi can I be assigned this issue? I've already got linfa-linear to work, and started to work on linfa-logistic, but it seems like argmin massively changed its trait structure and no longer has directly declared operators(ArgminSub,ArgminAdd, etc.). There is the Operator trait that might be relevant? but I'm not sure how to propagate it and if it would break things. I'm new to Rust but want to help! Thanks!

JadenMajid commented 1 year ago

whoops, looks like I got ahead of myself, I didn't get linfa-linear working, sorry about that

neoneye commented 11 months ago

Thanks for fixing this. Much appreciated.

Here is my commit where I migrate to the newest linfa version. It was really tricky. I had a wrong assumption about Cargo.toml, when specifying version="*", that the newest version would be used, but instead I ended up with a mix of both the newest linfa and an older linfa, causing weird error messages. These were the versions I had:

Setting the same version for all the linfa packages, and now it compiles fine. Had I done that from the beginning, then it would have been easy to fix.