Skip to content

Conversation

@chriselrod
Copy link
Contributor

This PR offers a bad API that we probably don't want to commit to.

I still need to test whether this actually works, e.g. if turning all of them off reduces precompile time.
Maybe much of the work here should be done outside of the SnoopPrecompile.@precompile_all_calls block?

@ChrisRackauckas
Copy link
Member

I think what we want is the type choices to be done at the DiffEqBase level, and each of the solver packages then have solver preferences, and the combination then is precompiled in each solver package. DiffEqBase.jl then could define a const for the type arguments it wants.

@chriselrod
Copy link
Contributor Author

For example, currently it precompiles Rodas4() and Rodas4(autodiff = false) by default, but not Rodas4(chunk_size = 1) or Rodas4(chunk_size = Val{1}()), but you should precompile all of these by setting Preferences.@set_preference!("Rodas4" => "true;true;true;true").


solver_list = []
for (solvername, solver) in nonstiff_solver_options
if Preferences.@load_preference(solvername, "true") == "true"
Copy link
Member

Choose a reason for hiding this comment

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

A bit simpler would be

Suggested change
if Preferences.@load_preference(solvername, "true") == "true"
if Preferences.@load_preference(solvername, true)

or

Suggested change
if Preferences.@load_preference(solvername, "true") == "true"
if Preferences.@load_preference(solvername, true) === true

Copy link
Contributor Author

@chriselrod chriselrod Aug 22, 2022

Choose a reason for hiding this comment

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

I haven't used or experimented with preferences much, but the README gave me the impression it could only load/save strings, while default could be anything.

Copy link
Member

Choose a reason for hiding this comment

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

ForwardDiff e.g. uses Bool and Int: https://github.com/JuliaDiff/ForwardDiff.jl/blob/61e4dd486a67cd0bdc6f61e58e452afcea388b7c/src/prelude.jl#L2-L3 I think everything that is supported by TOML might work (e.g., tuples didn't work but arrays did).

Copy link
Contributor Author

@chriselrod chriselrod Aug 22, 2022

Choose a reason for hiding this comment

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

Okay, how can one actually set these preferences to try without deving ForwardDiff?
My understanding is that @set_preferences! sets preferences with respect to the module in which it is located, meaning there has to be some function within ForwardDiff that includes @set_preferences!. As there are not, it is impossible to actually set?

Copy link
Member

Choose a reason for hiding this comment

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

@set_preferences! uses the current module but users would want to use set_preferences!: https://juliadiff.org/ForwardDiff.jl/dev/user/advanced/#Fixing-NaN/Inf-Issues There the first argument is the module or the UUID of the project for which you want to set preferences.

Copy link
Contributor Author

@chriselrod chriselrod Aug 22, 2022

Choose a reason for hiding this comment

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

Importantly (for the diffeq ecosystem), it also accepts the uuid, thus we can set_preferences! without first loading the module.

end
end
for (solvername, default, solvertype) in stiff_solver_options
options = split(Preferences.@load_preference(solvername, default), ';')
Copy link
Member

Choose a reason for hiding this comment

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

A bit simpler here would also be

Suggested change
options = split(Preferences.@load_preference(solvername, default), ';')
options = Preferences.@load_preference(solvername, default)

and use defaults of e.g. [true, false, false, true].

Comment on lines +254 to +255
Preferences.@load_preference("Float64", "true") == "true" && push!(eltypes, Float64)
Preferences.@load_preference("Float32", "false") == "true" && push!(eltypes, Float64)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Preferences.@load_preference("Float64", "true") == "true" && push!(eltypes, Float64)
Preferences.@load_preference("Float32", "false") == "true" && push!(eltypes, Float64)
Preferences.@load_preference("Float64", true) === true && push!(eltypes, Float64)
Preferences.@load_preference("Float32", false) === true && push!(eltypes, Float64)

@chriselrod chriselrod closed this Aug 22, 2022
@chriselrod chriselrod deleted the precomppreferences branch August 22, 2022 18:36
@chriselrod
Copy link
Contributor Author

I'm not a fan of Preferences.jl. I've wanted to use it a few times, but it always seems clunky, and never seems like the correct thing to do.
I think setting ENV is much more ergonomic, and is probably the behavior we'd prefer here.
Changing ENV variables doesn't trigger recompilation.
But we wouldn't want to trigger recompilation here if someone turned off the compilation of some methods (only if they were turned on).

That said, if we go with Preferences, we should not have the same package own the preferences as does the precompilation.

That is because Preferences seem to be bound to modules, and having to precompile so you can change a preference and recompile again is less than ideal.

@ChrisRackauckas
Copy link
Member

Note SciML/SciMLBase.jl#228

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