JuliaSmoothOptimizers / FluxNLPModels.jl

Other
6 stars 2 forks source link

#15 and #4 is addressed here #25

Closed farhadrclass closed 8 months ago

farhadrclass commented 8 months ago

I had also updated the Flux and therefore some updates are due to that

farhadrclass commented 8 months ago

@dpo, @d-monnet and @tmigot to review?

farhadrclass commented 8 months ago

It will error out since we needed to use Julia 1.9 for Flux I believe we need to update the CI ?

farhadrclass commented 8 months ago

Hi @tmigot

Thank you for the review.

The reason for combining the 4 and 15 bugs are that they are the same thing, one is asking to have unit test for different precision and the other one is mentioning that Float16 is not supported.

The reason we needed Julia 1.9+ is that we need Flux 13 and or 14th since they added support for changing parameters type on the run time and they require Julia 1.9 (Also Float16 is hardware supported :) )

Lastly I had a question on how to fix the errors we are getting for CI and Documentation ?

tmigot commented 8 months ago

Hi @tmigot

Thank you for the review.

The reason for combining the 4 and 15 bugs are that they are the same thing, one is asking to have unit test for different precision and the other one is mentioning that Float16 is not supported.

The reason we needed Julia 1.9+ is that we need Flux 13 and or 14th since they added support for changing parameters type on the run time and they require Julia 1.9 (Also Float16 is hardware supported :) )

Lastly I had a question on how to fix the errors we are getting for CI and Documentation ?

Thanks for the additional explanations. Still, I think you should first open a PR to drop Julia 1.6 (in the Project.toml and in all the config files) that will fix the CI thing.

tmigot commented 8 months ago

The documentation fails because you drop support of Flux 0.13 while it is the version used for the doc https://github.com/JuliaSmoothOptimizers/FluxNLPModels.jl/blob/main/docs/Project.toml

farhadrclass commented 8 months ago

@tmigot I added a new PR #26 for Julia and flux update, once that resolve, I would rebase this

farhadrclass commented 8 months ago

@tmigot I rebased it and now waiting for it to run the CI

Let me know if anything Also some notes on Zygote: due to some reason we need Zygote 0.6.6 since Flux is not using the most updated Zygote

farhadrclass commented 8 months ago

@tmigot I added some multiple dispatch but not sure about all of them, what do you think

farhadrclass commented 8 months ago

@tmigot I added your suggestions, except the multip-dispatch which I explained. Once this is good to go we can start the other prs

tmigot commented 8 months ago

@farhadrclass please use ''squash and merge'' button so that we try to keep a clean history of commit (hère 18commits have been merged while it is only a small change)

farhadrclass commented 8 months ago

do you want me to do it now or in the future PR?

farhadrclass commented 8 months ago

I do not know how to do it after you merge btw @tmigot

tmigot commented 7 months ago

I do not know how to do it after you merge btw @tmigot

This is done :). It is better to "squash and merge" to keep a clean history