JuliaNLSolvers / Optim.jl

Optimization functions for Julia
Other
1.1k stars 214 forks source link

Fix _init_identity_matrix in bfgs.jl #1089

Open odow opened 3 months ago

odow commented 3 months ago

x-ref https://discourse.julialang.org/t/error-loaderror-methoderror-no-method-matching-init-identity-matrix-componentvector-float32-vector-float32-tuple-axis-float64/112211

I don't really know the implications of this, but I assume this is the fix.

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 85.43%. Comparing base (7cc8328) to head (84ca7f0).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1089 +/- ## ======================================= Coverage 85.43% 85.43% ======================================= Files 45 45 Lines 3276 3276 ======================================= Hits 2799 2799 Misses 477 477 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

devmotion commented 3 months ago

I think it might be more consistent to apply the pattern in https://github.com/JuliaNLSolvers/Optim.jl/blob/7cc8328a590a0c815e23005321459712382b5b42/src/multivariate/solvers/first_order/bfgs.jl#L81 to https://github.com/JuliaNLSolvers/Optim.jl/blob/7cc8328a590a0c815e23005321459712382b5b42/src/utilities/perform_linesearch.jl#L18.

Generally, isn't the error caused by an incorrectly typed initial_stepnorm provided by the user? If you deal with Float32 data, you can avoid the problem by specifying also initial_stepnorm as a Float32. That being said, I guess https://github.com/JuliaNLSolvers/Optim.jl/blob/7cc8328a590a0c815e23005321459712382b5b42/src/multivariate/solvers/first_order/bfgs.jl#L81 makes it a bit more convenient for users.

pkofod commented 2 months ago

I think it might be more consistent to apply the pattern in

I agree

pkofod commented 2 months ago

Generally, isn't the error caused by an incorrectly typed initial_stepnorm provided by the user? If you deal with Float32 data, you can avoid the problem by specifying also initial_stepnorm as a Float32. That being said, I guess

Sure, but usually I think it should be enough to supply x0 as the type you want even if there might be some cases where it's harder to prepare for that.. but I think we can apply the "correct" pattern here for the user..