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

Section sampler #279

Merged
merged 26 commits into from
Jul 29, 2021
Merged

Section sampler #279

merged 26 commits into from
Jul 29, 2021

Conversation

marcoq
Copy link
Contributor

@marcoq marcoq commented Jun 8, 2021

In surrogate-based identification and control,
optimization can alternate between unconstrained sampling
in the full-dimensional parameter space,
and sampling constrained on specific sections (e.g. a planes in a 3D volume),

A SectionSampler allows sampling and optimizing
on a subset of 'free' dimensions while keeping 'fixed' ones constrained.

The sampler is defined as in e.g.

section_sampler_y_is_10 = SectionSample(
    [NaN64, NaN64, 10.0, 10.0],
    Surrogates.UniformSample())

where the first argument is a Vector{T}
in which numbers are fixed coordinates
and NaNs correspond to free dimensions,
and the second argument is a SamplingAlgorithm
which is used to sample in the free dimensions.

@ChrisRackauckas
Copy link
Member

Hard to tell if that's a related CI failure from a quick glance.

@marcoq
Copy link
Contributor Author

marcoq commented Jun 8, 2021

Not sure how to solve this: the point of failure of the tests at optimization.jl:102 is exercising a NeuralSurrogate, which is untouched in the Commits for this Pull Request.

Significantly, the issue seems to reproduce on the master branch, if Zygote is in v0.6.12.
NeuralSurrogate tests in test/neuralSurrogate.jl seem to fail as well with the same Zygote.OneElement-related error.
With Zygote v0.6.10 the tests were fine.

Having a deeper look to verify whether Zygote v0.6.12 tests pass on my machine.

@mcabbott
Copy link

mcabbott commented Jun 9, 2021

This looks like it was caused by FluxML/Zygote.jl#962. That makes the gradient of getindex a special immutable type, which probably should not be saved in Params as things expect mutability.

So my guess is that the fix is something like this:

Base.setindex!(dict::IdDict, dx::Zygote.OneElement, x) = dict[x] = collect(dx)

@ChrisRackauckas
Copy link
Member

Yeah the user probably shouldn't be exposed to that type. That seems leaky.

@mcabbott
Copy link

mcabbott commented Jun 9, 2021

Yes, I mean as a quick check! If this fixes it, then it's a Zygote bug, and this should become a PR, with tests...

@marcoq
Copy link
Contributor Author

marcoq commented Jun 9, 2021

Added the workaround suggested by Michael Abbott

Base.setindex!(dict::IdDict, dx::Zygote.OneElement, x) = dict[x] = collect(dx)

to src/NeuralSurrogates.jl.

The tests now pass with a local environment with the latest version of the libraries.

@codecov
Copy link

codecov bot commented Jun 9, 2021

Codecov Report

Merging #279 (096f369) into master (24bc905) will increase coverage by 0.17%.
The diff coverage is 84.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #279      +/-   ##
==========================================
+ Coverage   80.61%   80.79%   +0.17%     
==========================================
  Files          19       19              
  Lines        2229     2301      +72     
==========================================
+ Hits         1797     1859      +62     
- Misses        432      442      +10     
Impacted Files Coverage Δ
src/NeuralSurrogate.jl 100.00% <ø> (ø)
src/Surrogates.jl 28.00% <ø> (ø)
src/Optimization.jl 72.86% <81.96%> (+0.62%) ⬆️
src/Sampling.jl 99.18% <93.33%> (-0.82%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24bc905...096f369. Read the comment docs.

@ChrisRackauckas
Copy link
Member

👍 upstream it to Zygote.

@marcoq
Copy link
Contributor Author

marcoq commented Jul 29, 2021

Flux.jl was pushed 6 days ago to v0.12.6:

I hope we can make it with the merge :-)

@marcoq
Copy link
Contributor Author

marcoq commented Jul 29, 2021

Just eliminated the stale "workaround" that was still hanging in the code...

@marcoq
Copy link
Contributor Author

marcoq commented Jul 29, 2021

It looks like the coverage went down 0.09%, which is preventing one last check from being passed.

Looking into it....

@marcoq
Copy link
Contributor Author

marcoq commented Jul 29, 2021

Simplified a return statement.

Also tried compressing comments - which should not count in coverage measurements anyway...

Simplified return statement
@marcoq
Copy link
Contributor Author

marcoq commented Jul 29, 2021

All checks passed! :-)

@@ -34,6 +34,12 @@ LowDiscrepancySample{T}
sample(n,lb,ub,S::LowDiscrepancySample)
```

* Sample on section
```@docs
SectionSample
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't have a docstring?

@@ -1711,3 +1711,82 @@ end
end
return pareto_set,pareto_front
end


function surrogate_optimize(obj::Function,::EI,lb,ub,krig,sample_type::SectionSample;maxiters=100,num_new_samples=100)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This specialization may need docs.

@ChrisRackauckas
Copy link
Member

Nice! Follow up with a bit more docs?

@ChrisRackauckas ChrisRackauckas merged commit 73fd3bf into SciML:master Jul 29, 2021
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.

3 participants