JuliaSmoothOptimizers / SolverTools.jl

Tools for developing nonlinear optimization solvers.
Other
26 stars 18 forks source link

Move `compute_As_slope_qs!` in this package #196

Closed tmigot closed 9 months ago

tmigot commented 1 year ago

https://github.com/JuliaSmoothOptimizers/JSOSolvers.jl/blob/5c5294cc90036d606d6aa204d43f03654e61f3e5/src/tronls.jl#L8

This is the same as https://github.com/JuliaSmoothOptimizers/SolverTools.jl/blob/72c5ce136377b6b2c0dab043ee6a2a74379800e9/src/auxiliary/bounds.jl#L93 but used in NLS solvers.

Jay-sanjay commented 9 months ago

Hi sir, as far as I see you want to move this function to SolverTools.jl package https://github.com/JuliaSmoothOptimizers/JSOSolvers.jl/blob/5c5294cc90036d606d6aa204d43f03654e61f3e5/src/tronls.jl#L12C1-L23C1

Sir , could you please assist me in solving this issue , I would be happy to contribute

tmigot commented 9 months ago

Hi @Jay-sanjay ! Thank you for showing a great motivation, and sorry for the delays (I noticed you answered to other issues as well and I am trying to catch-up).

Feel free to open a pull request to fix it and ask reviews from me and maybe @amontoison . Please pay attention to adding unit tests and doc, thank you.

After adding the function in SolverTools.jl, we will make a new release and remove them from JSSolvers.jl.

amontoison commented 9 months ago

@Jay-sanjay Don't hesitate to ping me for a review.

Jay-sanjay commented 9 months ago

@amontoison sir please correct if I am wrong, both compute_As_slope_qs! and compute_Hs_slope_qs! functions would function exactly the same way and would give same final outputs if same inputs are provided ...?

amontoison commented 9 months ago

@Jay-sanjay No, if you use the same arguments in both functions, you will have different outputs.

function compute_As_slope_qs!(
  Hs::AbstractVector{T},  # argument As renamed
  H::Union{AbstractMatrix, AbstractLinearOperator},  # argument A renamed
  s::AbstractVector{T},
  g::AbstractVector{T},  # argument Fx renamed
) where {T <: Real}
  mul!(Hs, H, s)
  slope = dot(Hs, g)
  qs = dot(Hs, Hs) / 2 + slope
  return slope, qs
end

function compute_Hs_slope_qs!(
  Hs::AbstractVector{T},
  H::Union{AbstractMatrix, AbstractLinearOperator},
  s::AbstractVector{T},
  g::AbstractVector{T},
) where {T <: Real}
  mul!(Hs, H, s)
  slope = dot(g, s)
  qs = dot(s, Hs) / 2 + slope
  return slope, qs
end
Jay-sanjay commented 9 months ago

@amontoison Sir, here is the PR https://github.com/JuliaSmoothOptimizers/SolverTools.jl/pull/204 , you can review it

Jay-sanjay commented 9 months ago

Hi @Jay-sanjay ! Thank you for showing a great motivation, and sorry for the delays (I noticed you answered to other issues as well and I am trying to catch-up).

Feel free to open a pull request to fix it and ask reviews from me and maybe @amontoison . Please pay attention to adding unit tests and doc, thank you.

After adding the function in SolverTools.jl, we will make a new release and remove them from JSSolvers.jl.

@tmigot Sir can I work on removing the same from JSSolvers.jl

amontoison commented 9 months ago

Yes, you can :) Thanks for your help!

tmigot commented 9 months ago

Hey @Jay-sanjay , Thank you for your contribution. There has been a new release of SolverTools.jl (v0.8.6), so you should be able to remove the function from JSOSolvers now.

Jay-sanjay commented 8 months ago

Hello sir I was working on removing the compute_As_slope_qs function. But , I see that it is used in various other functions here https://github.com/JuliaSmoothOptimizers/JSOSolvers.jl/blob/5c5294cc90036d606d6aa204d43f03654e61f3e5/src/tronls.jl#L416C1-L433C1 so what to replace over there....?

amontoison commented 8 months ago

compute_As_slope_qs! is implemented here so you can remove it from JSOSolvers.jl because SolverTools.jl is a dependency of JSOSolvers.jl. If the function is exported here it should work fine.