General refactoring and cleanup in preparation for release v0.2#14
General refactoring and cleanup in preparation for release v0.2#14bgroenks96 wants to merge 28 commits intomainfrom
Conversation
22fa9ba to
07ebc1c
Compare
1e45a4a to
aa85100
Compare
|
The following code is not type stable: An alternate pattern is: |
result should have its own type. |
|
I think my biggest comment overall is actually about types. In many places i feel types are either under-restricted (e.g., above). This can lead to degraded performance. I think we should lean on Julia's type promotion system. Here is an example patter for |
|
Hi @rsenne, thanks a lot for the feedback! Much appreciated. I assure you that I am aware of type stability concerns and how Julia's type system works. As a general rule, if a Fully type-parameterized structs do not come for free. They increase type-inference/compile-time as well as type complexity, making debugging (e.g. stack traces and error messages) messier and thus pose usability concerns. Furthermore, the presence of an untyped or abstract typed field is not automatically a cause of type instability. It depends on how/where it is used; e.g. in this example that you pointed out: mutable struct SimulatorInferenceSolution{algType,probType,storageType}
prob::probType
alg::algType
storage::storageType
result::Any
end
Similarly here struct SimulationArrayStorage{inputType, outputType, metaType} <: SimulationData{inputType, outputType}
inputs::Vector{inputType}
outputs::Vector{outputType}
metadata::Vector{metaType}
end
SimulationArrayStorage(;
input_type::Type = Any,
output_type::Type = Any,
metadata_type::Type = Any
) = SimulationArrayStorage(input_type[], output_type[], metadata_type[])the use of In general, however, I would not be terribly worried about degraded performance at the level of the inference algorithm. For SBI applications, it's rarely the bottleneck, at least in my experience. Usually, it's the simulator that you need to worry about. So heeding the preeminent words of Donald Knuth -- "Premature optimization is the root of evil" -- I would make sure that any additional type complexity is justified by a specific, measurable performance issue due to type inference failure. |
|
That being said, you are absolutely right that we need more generic types for compatibility with GPU arrays and alternative number formats like |
|
Hi @bgroenks96!
Sorry! I wasn't trying to imply that you weren't aware, this is just what I saw on a cursory glance and you obviously know this code far better than I do. Your reasoning makes sense to me, so i agree that this isn't as big of a concern as my initial message suggested |
|
No worries, just wanted to make sure we're on the same page. Note that 3ed2928 adds generic types to the ensemble solvers and adds a type parameter for |
The primary goal of this PR is to make the package functional again with up-to-date dependencies (the v0.1.x versions currently have hard compat bounds that cause version conflicts for some package combinations) and to make several necessary changes to the API that should greatly improve clarity and usability.
Breaking changes
DiffEqBase,Zygote, andAbstractGPs.Emulatorsmodule. This has always been arguably out-of-scope of theSimulationBasedInferencepackage and increased the footprint by adding dependencies. My current plan is to re-introduce this code as a separate source module in the repository with the goal of eventually registering it as a standalone package. Also removes the GP likelihood for now.SimulatorForwardProblemfrom the SciML problem interface by allowing it to directly handle arbitrary simulator functions/types.SimulatorObservableconstructor(s) to be more clear and concise, as well as enablingdo-syntax for the observable function.ensemble_solvewith a more general implementation forSimulatorForwardProblems that have matrix-valued parameters where columns correspond to parameter samples (ensemble members). This decouples the parallel/ensemble forward simulation code from the ensemble inference algorithms.SimulationArrayStorage; e.g. ensemble matrices are now stored directly rather than being flattened into the input/output arrays.SimulationPriors or can directly incorporateSimulatorForwardProbleminto a full probabilistic program on a case-by-case basis.Other changes
Simulatorinterface for defining external simulator types. TheSimulatortrait currently has three subtypes:ForwardMap,IterativeandDynamical. The interfaces forIterativeandDynamicalsimulators are extensions of theCommonSolveinterface also used bySimulatorForwardProblem.DiffEqBaseextension moduleThis PR is still a WIP and I will update this with further changes.