the various refactors collected in #52 are a good step towards taming the parameter spray.
however they attempt to preserve "legacy" behavior (#50 ) and the models aren't used in what might be called an intuitive way: ideally the models should be instantiated once and only once, and require no wrapping code for instantiation. then everywhere else should only depend on the models, rather than passing any individual parameters around.
so some examples
- this function should be removed, and only "pipeline_bin_new" should stay - we don't need a wrapping function that takes all the parameters and constructs the model "from legacy kwargs" - just accept the data and the config object:
- this module
init.py shouldn't need to exist, as ARParams can have this logic in its model validators, and DeconvBin should just accept a config object: https://github.com/Aharoni-Lab/indeca/blob/ab624b3d4c6e415a5d39e02ba059c2ed5766e98f/src/indeca/pipeline/init.py
DeconvBin should not be initialized with kwargs and then create a deconv.config.DeconvConfig object - it should just accept a DeconvConfig object. This one is especially egregious because there are two deconv configuration models - deconv.config.DeconvConfig and pipeline.config.DeconvStageConfig that have mostly the same parameters, and the execution logic goes
pipeline.pipeline_bin: pack kwargs into DeconvPipelineConfig (which contains DeconvStageConfig)binary_pursuit.pipeline_bin: unpack DeconvStageConfig (and an AR config var too?) into kwargsinitialize_deconvolvers: forward (some of??) the kwargs to DeconvBinDeconvBin: pack kwargs into DeconvConfig
instead there should be one model to configure deconvolution, pipeline_bin accepts the model, and the (sub)model is passed to all the inner methods that need it.
all of this means that we have to ensure that the same types and defaults are matched across several places, and each of those introduces opportunities for errors if the kwargs are not perfectly forwarded on. the purpose of using models to collect parameters is to not have to worry about passing so many parameters around, but in the current implementation the models multiply the number of times the parameters are passed around.
the various refactors collected in #52 are a good step towards taming the parameter spray.
however they attempt to preserve "legacy" behavior (#50 ) and the models aren't used in what might be called an intuitive way: ideally the models should be instantiated once and only once, and require no wrapping code for instantiation. then everywhere else should only depend on the models, rather than passing any individual parameters around.
so some examples
indeca/src/indeca/pipeline/pipeline.py
Line 28 in ab624b3
init.pyshouldn't need to exist, asARParamscan have this logic in its model validators, andDeconvBinshould just accept a config object: https://github.com/Aharoni-Lab/indeca/blob/ab624b3d4c6e415a5d39e02ba059c2ed5766e98f/src/indeca/pipeline/init.pyDeconvBinshould not be initialized with kwargs and then create adeconv.config.DeconvConfigobject - it should just accept aDeconvConfigobject. This one is especially egregious because there are two deconv configuration models -deconv.config.DeconvConfigandpipeline.config.DeconvStageConfigthat have mostly the same parameters, and the execution logic goespipeline.pipeline_bin: pack kwargs intoDeconvPipelineConfig(which containsDeconvStageConfig)binary_pursuit.pipeline_bin: unpackDeconvStageConfig(and an AR config var too?) into kwargsinitialize_deconvolvers: forward (some of??) the kwargs toDeconvBinDeconvBin: pack kwargs intoDeconvConfiginstead there should be one model to configure deconvolution,
pipeline_binaccepts the model, and the (sub)model is passed to all the inner methods that need it.indeca/src/indeca/core/deconv/deconv.py
Line 36 in ab624b3
all of this means that we have to ensure that the same types and defaults are matched across several places, and each of those introduces opportunities for errors if the kwargs are not perfectly forwarded on. the purpose of using models to collect parameters is to not have to worry about passing so many parameters around, but in the current implementation the models multiply the number of times the parameters are passed around.