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

Breaking tests with Complex input #775

Closed
wants to merge 2 commits into from

Conversation

thisrod
Copy link
Contributor

@thisrod thisrod commented Jan 16, 2020

This adds two tests, that currently throw errors for the following reasons:

  1. (I think) The show or repr method for solutions tries to @printf complex numbers.

  2. The default solver attempts to sort complex numbers.

@pkofod
Copy link
Member

pkofod commented Jan 22, 2020

Interesting :) Good catach... I will think about a better way of printing complexs (or just not print these)

@antoine-levitt
Copy link
Contributor

Can this be merged?

…lvers#775)

This is a minimal fix.  Previous versions iterated over the minimizer,
and displayed the first few elements compactly with @printf "%.2e".
The @printf has been replaced with repr(_, context=:compact =>
true).  Ideally, the whole minimizer could be formatted with
repr(minimizer(r), context=:compact => true).  However, this currently
prints every element of the minimizer.
@thisrod
Copy link
Contributor Author

thisrod commented Feb 28, 2020

I've fixed the first one, I think.

@thisrod
Copy link
Contributor Author

thisrod commented Mar 1, 2020

I'm looking at point 2. The error occurs at line 156. This assumes that the type of the minimum, returned by the cost function, is the same as the eltype of the minimizer. The error is caught at line 167, which sorts an array that is supposed to hold real costs, but has inherited the complex eltype of the minimizer.

function initial_state(method::NelderMead, options, d, initial_x)
T = eltype(initial_x)
n = length(initial_x)
m = n + 1
simplex = simplexer(method.initial_simplex, initial_x)
f_simplex = zeros(T, m)
value!!(d, first(simplex))
@inbounds for i in 1:length(simplex)
f_simplex[i] = value(d, simplex[i])
end
# Get the indices that correspond to the ordering of the f values
# at the vertices. i_order[1] is the index in the simplex of the vertex
# with the lowest function value, and i_order[end] is the index in the
# simplex of the vertex with the highest function value
i_order = sortperm(f_simplex)

I presume that the code is supposed to handle a cost function that returns any subtype of Real, though I don't fully understand the design intent here. Is there a reliable way to determine the type of the minimum? I'd guess that a reliable and extensible implementation of Optim will need one. Should be a method to return that type for a given problem?

A quick fix would be add, after line 152, something like if T <: Complex; T = type_parameter_of(T); end. I don't know the idiomatic way to do that in Julia. It can be done by defining a function that dispatches on Complex{T} and T <: Real, but that seems a bit clunky.

@thisrod
Copy link
Contributor Author

thisrod commented Mar 4, 2020

At a higher level, I think there's a choice to be made between the following designs:

  1. Every gradient-free solver is implemented to handle complex minimizers, or

  2. Gradient-free solvers are only implemented for real minimizers, and there is a generic constructor method for complex minimizers that splits the problem into its real and imaginary parts then converts the solution back to complex.

@pkofod
Copy link
Member

pkofod commented Apr 2, 2020

Yeah, I think maybe gradient free solvers will have to use the splitting and or special cases for the complex case.The problem with nelder mead (if I fixed the type stuff you found above) for example is that the simplex will be of order two times too low, and it will not be able to disentangle the real and imaginary part the way it's written right now.

@thisrod
Copy link
Contributor Author

thisrod commented Apr 2, 2020

I think maybe gradient free solvers will have to use the splitting and or special cases for the complex case.

Then the next step might be to merge the display fix, and raise the gradient free solvers as an issue that will take longer to fix.

What's the best way to rebase this PR onto the master branch? Should I just do it and force push to my github? I'll add an @test_broken to make Travis happy.

@thisrod
Copy link
Contributor Author

thisrod commented Jun 24, 2020

I have a plan for zeroth order complex. Please tell me if there is an obvious problem or better way.

  1. In NLSolversBase, implement ComplexNonDifferentiable. This wraps the NonDifferentiable methods with Complex, to combine real and imaginary parts.

  2. In Optim, implement this pseudo code:

    function optimize(
    d::AbstractObjective,
    initial_x::AbstractArray{<:Complex},
    method::ZerothOrderOptimizer,
    options
    )
    c = ComplexObjective(d)
    # split initial_x into real and imaginary parts
    results = optimize(c, initial_parts, method, options)
    # combine real and imaginary results into complex
    results
    end

@thisrod thisrod closed this Oct 19, 2020
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.

None yet

3 participants