hartytp
commented
1 year ago Rescaling
Currently the model parameters are rescaled by the model instance during the fit process. This leads to a slightly awkward dance where we deep copy the model and is generally a bit unergonomic.
Related: estimate_parameters takes a model_parameters dictionary, which is just set to self.parameters but the calling fitter. It would be cleaner to let the estimator just mute self.model_parameters directly. This is a bit historical quirk from how rescaling used to be done. I haven't changed this yet because I still have some uncertainty about what the right overall solution is.
Things would be quite a bit simpler if the fitter could just rescale the model's parameter dictionary itself. One reason we moved away from that is that some models have "internal" parameters, which are not part of the parameters dictionary. This kind of situation typically arises when a model encapsulates one or more models. See, for example, the "inner" parameters from MappedModel's. A similar situation might arise, for example if one wanted to make a Rabi model for two qubits that internally calls the standard rabi model.
The current design essentially takes the maximally flexible approach here by letting the model take care of scaling however it wants. The downside of that is that it's probably also the maximally complicated approach and puts a burden on the model developer to think about how to rescale internal parameters rather than things "just working".
One solution would be to say that these kinds of use-cases are actually a bit niche and not worth the hassle of supporting. Maybe better, we can do an audit of use-cases and add first-class support for specific use-cases, but not be fully general. For example, we could make the notion of an "internal parameter" a first-class part of the API and have the fitter rescale those at the same time as the normal model functions. We could maybe also add some API to help (re)binding parameters from "internal models" to the outer model. That's probably not a huge amount of work but needs thought about what we want to achieve.
Somewhat related: it might be cleaner to make the attributes of ModelParameter properties rather than directly exposing the values. This might let us handle rescaling in a more ergonomic fashion. e.g. rescale just sets a scale factor which is internal to the model. We can also have a use_rescale or something attribute which modifies how the properties function. That would clean up the "unscaling" process we need to do at the end of the fit to get values out.
This package is now at a stage where the key features are implemented and working and it's starting to get battle-tested. I'm opening this issue as a place to record things (primarily affecting internal API and dataflow within the library) I would have done differently if I were starting from scratch now. I've chosen not to fix these issues at this stage because the correct approach depends a bit on what the use-cases for the package turn out to be and what features we want to support. I now want to give it a bit of time for the dust to settle before coming back and deciding on what to do about them...