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

add smoke test for ffjord #621

Merged
merged 4 commits into from
Sep 23, 2021
Merged

add smoke test for ffjord #621

merged 4 commits into from
Sep 23, 2021

Conversation

prbzrg
Copy link
Member

@prbzrg prbzrg commented Sep 17, 2021

I just added some smoke tests for ffjord to reveal errors related to some adtypes
It reveals errors that reported in #588, #610, #615, #624
This PR can be merged when these errors fixed

@ChrisRackauckas
Copy link
Member

Instead, just set @test_broken for now.

@prbzrg
Copy link
Member Author

prbzrg commented Sep 17, 2021

Should I expand the tests and unroll the loop? (to use both @test and @test_broken)

@ChrisRackauckas
Copy link
Member

Are all of the tests broken?

@prbzrg
Copy link
Member Author

prbzrg commented Sep 20, 2021

Regardless of adtype, using

regularize=true & monte_carlo=false

make errors and using

GalacticOptim.AutoForwardDiff()
GalacticOptim.AutoReverseDiff()
GalacticOptim.AutoTracker()

as adtype is also broken.
Overall, 14 of 20 tests are broken.

@ChrisRackauckas
Copy link
Member

as adtype is also broken.

What do you mean?

@prbzrg
Copy link
Member Author

prbzrg commented Sep 21, 2021

In each #610, #615, #624, I tested the same code with different adtype and reported errors.
AutoZygote and AutoFiniteDiff works fine, but other adtypes make different errors.

@ChrisRackauckas
Copy link
Member

oh with FFJORD, yes.

@ChrisRackauckas
Copy link
Member

It looks like this still isn't catching some of the errors?

@prbzrg
Copy link
Member Author

prbzrg commented Sep 22, 2021

If we only use @test, it could reproduce all four issues, but also make testing fail, you can see the errors in CI of first commit:
https://github.com/SciML/DiffEqFlux.jl/runs/3628543892?check_suite_focus=true#step:6:312
https://github.com/SciML/DiffEqFlux.jl/runs/3628543892?check_suite_focus=true#step:6:636
https://github.com/SciML/DiffEqFlux.jl/runs/3628543892?check_suite_focus=true#step:6:969
https://github.com/SciML/DiffEqFlux.jl/runs/3628543892?check_suite_focus=true#step:6:1350

I think we have two choices:

  1. Only use @test and wait until all four issues fixed to merge this.
  2. Use both @test and @test_broken by unrolling the loop. We can merge this sooner, but it doesn't log errors to CI, and it increases line of codes (more code to read and review)

I don't know whether there is any other choices. By second one, if someone wants to fix the errors, just changes it to @test and investigate the problems, but I prefer shorter codes.
Which way should we go?

@ChrisRackauckas
Copy link
Member

(2) is a better way to go, because it's always better to fix and change from broken than have things documented in unmerged PRs.

@prbzrg
Copy link
Member Author

prbzrg commented Sep 23, 2021

OK, I made the loop unrolled and also converted section comments to testsets.
Furthermore, I notice something; there is a test for FFJORD with regularizers

###
# test for default multivariate distribution and FFJORD with regularizers
###
nn = Chain(Dense(1, 1, tanh))
data_dist = Beta(7, 7)
data_train = Float32.(rand(data_dist, 1, 100))
tspan = (0.0f0, 1.0f0)
ffjord_test = FFJORD(nn, tspan, Tsit5())
function loss_adjoint(θ)
logpx, λ₁, λ₂ = ffjord_test(data_train, θ, true)
return mean(@. -logpx + 0.1 * λ₁ + λ₂)
end
optfunc = GalacticOptim.OptimizationFunction((x, p) -> loss_adjoint(x), GalacticOptim.AutoZygote())
optprob = GalacticOptim.OptimizationProblem(optfunc, 0.01f0 .* ffjord_test.p)
res = GalacticOptim.solve(optprob, ADAM(0.1), cb=cb, maxiters=300)
θopt = res.minimizer
data_validate = Float32.(rand(data_dist, 1, 100))
actual_pdf = pdf.(data_dist, data_validate)
learned_pdf = exp.(ffjord_test(data_validate, θopt; monte_carlo=false)[1])
@test totalvariation(learned_pdf, actual_pdf) / size(data_validate, 2) < 0.40

but it doesn't use regularize=true!

@prbzrg
Copy link
Member Author

prbzrg commented Sep 23, 2021

I want to update other tests to more look like tests of this PR:

  1. Use Float32 distribution instead of converting samples
  2. Use DiffEqFlux.sciml_train instead of OptimizationFunction -> OptimizationProblem -> solve

Can I update other tests in this PR?

@prbzrg
Copy link
Member Author

prbzrg commented Sep 23, 2021

I made the updates. If you disagree, I will move them to new PR.

@prbzrg
Copy link
Member Author

prbzrg commented Sep 23, 2021

Tests are failing. I just undo two last commits and will change other tests in a new PR.

@ChrisRackauckas ChrisRackauckas merged commit d21eb9d into SciML:master Sep 23, 2021
@ChrisRackauckas
Copy link
Member

Thanks!

@prbzrg prbzrg deleted the smoke-test branch September 26, 2021 15:00
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.

2 participants