ModelingToolkit.jl
ModelingToolkit.jl copied to clipboard
SciML
Change the default for `istunable` to true.
MTKParameters considers parameters as tunable unless marked as not tunable from what I understand, so it would be nice for this to agree with what istunable reports.
I'm a bit sceptical to this change for two reasons
- it's a breaking change
- for non-toy models, most parameters just aren't tunable, or are known exactly from CAD drawings etc. Treating all parameters as tunable in such a situation will lead to a model that deviates significantly from the actual system due to extreme overfitting. Keep in mind, realistic models may have 100s to 1000s of parameters.
- for non-toy models, most parameters just aren't tunable, or are known exactly from CAD drawings etc. Treating all parameters as tunable in such a situation will lead to a model that deviates significantly from the actual system due to extreme overfitting. Keep in mind, realistic models may have 100s to 1000s of parameters.
I agree here. Also, the couple of large models I've seen, one can get good fits by tuning just a small number of parameters out of the total, in the order of 1-5% of the total number of params, so the responsibility should be on the user to mark these as tunable.
In this case should MTKParameters change its behavior? cc @AayushSabharwal
for non-toy models, most parameters just aren't tunable, or are known exactly from CAD drawings etc.
That's just not true in general. I can list out a bunch of domains for which that is not true, and I can list out a bunch of domains for which is true. The real issue here is that it is domain dependent. In the ecosystem, it comes out to more like 70% of folks expect things to be tunable by default, 30% other way around. A large portion of that is due to biological and pharmacological domains which have mechanistically-based models in the thousands of parameters. So it is simply false to claim this, and a real solution needs to take the actual landscape into account.
What it probably needs is 3-valued logic Union{Nothing,Bool} where nothing implies it should use whatever the default is in the ODEProblem construction, and ::Bool is then an override of that default. Then in the ODEProblem constructor we should have a default_tunable = true to change the interpretation of nothing.
The next question of course is why the interpretation of nothing should default to true instead of false. But this comes back to one of the core principles of SciML which is "the simple thing should work and the extra burden should be on the experts". In this case, if someone is to the point where they know that they want to tune a subset of the parameters, then they are already the kind of person who would look up tunable and thus the documentation around tunable would be a good place to mention the option for default_tunable = false. In the other case, people who weren't thinking about subsetting also have a burden of knowledge, and this is also people building simpler models, which is simply not the correct direction for burden of knowledge.
it's a breaking change
Given that it was previously unused in the ecosystem and we just had a breaking change that essentially no one can install yet (not without a few more bumps around the ecosystem), I think it's fine to get this into the v9 as a late addition, as long as we get it in the next week (@AayushSabharwal).
There are two slightly different use cases being discussed here. When I added this metadata, "tunable" was intended to indicate that a parameter was free to be chosen by the user of a system in order to tune the system to their liking, such as controller parameters etc (indicated in the docs for tunable). This is different from calibrating a parameter to make the model fit data, what we otherwise call "calibration".
If the default is true, the metadata is only useful as an opt-out, while if the default is false, users can opt-in to have some parameters tunable. The functionality is already made to handle an optional default which can be specified when asking for tunable parameters, so the problem is not big in practice. https://github.com/SciML/ModelingToolkit.jl/blob/master/src/variables.jl#L260 https://github.com/SciML/ModelingToolkit.jl/blob/master/test/test_variable_metadata.jl#L71
Maybe it would make sense to introduce different metadata to indicate whether or not something can be adjusted to calibrate the model to data, and keep this a separate concept from a parameter being adjusted to meet a performance goal, such a controller parameters.
A third idea is to remove the metadata altogether. tunable = true/false is specified at the time of variable creation, which is not really when you want to specify this information in a lot of cases. Both JSMO and JSControl has opted to specify parameters to optimize over explicitly when creating the optimization problem, which is generally a more flexible approach. Whether or not a parameter is tunable/calibratable depends on how the model/component is to be used right now, and it doesn't make sense to, e.g., specify this in the MTKstdlib
Maybe it would make sense to introduce different metadata to indicate whether or not something can be adjusted to calibrate the model to data, and keep this a separate concept from a parameter being adjusted to meet a performance goal, such a controller parameters.
The functional difference is whether they show up as Tunable for autodiff.
A third idea is to remove the metadata altogether. tunable = true/false is specified at the time of variable creation, which is not really when you want to specify this information in a lot of cases. Both JSMO and JSControl has opted to specify parameters to optimize over explicitly when creating the optimization problem, which is generally a more flexible approach. Whether or not a parameter is tunable/calibratable depends on how the model/component is to be used right now, and it doesn't make sense to, e.g., specify this in the MTKstdlib
It's already a system level property, not a variable level property. Hence the 3 valued logic (which already exists for the most part)