-
Notifications
You must be signed in to change notification settings - Fork 1
Description
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:
indeca/src/indeca/pipeline/pipeline.py
Line 28 in ab624b3
def pipeline_bin( - this module
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.py DeconvBinshould 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 intoDeconvConfig
instead 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
def __init__(
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.