search

ICB-DCM / PESTO

PESTO: Parameter EStimation TOolbox, Bioinformatics, btx676, 2017.
https://doi.org/10.1093/bioinformatics/btx676
BSD 3-Clause "New" or "Revised" License
26 stars 12 forks source link

Duplicate properties in PestoOptions and PestoSamplingOptions #77

Closed dweindl closed 7 years ago

dweindl commented 7 years ago

The obj_type, rndSeed and mode properties exist in both PestoOptions and PestoSamplingOptions. The old discussion. Not urgent, but we should decide at some point how we want to handle this.

paulstapor commented 7 years ago

My favorite for a long term solution: PestoSamplingOptions is contained in PestoOptions, and PestoOptions is handed over to getParameterSamples

BBallnus commented 7 years ago

PestoOptions and PestoSamplingOptions are entirely "orthogonal" now. Since I was told, that we are planning to do an options class for each type of method (optimization, profiles, sampling, visualization etc) this is not a problem from my perspective

FFroehlich commented 7 years ago

Aren't PestoOptions.rng and PestoSamplingOptions.RndSeed specifying the same thing? You might run into trouble there in settings where you set the random seed first then perform getMultiStarts and then do the sampling (I do not know whether this is currently possible). This is because the random seed has a global effect but you are trying to specify it (object-) local. Thus, I would recommend also making this a global Pesto option in PestoOptions and remove rndSeed.

dweindl commented 7 years ago

@FFroehlich I am not sure how helpful this option is anyways. Wouldn't it be better to just completely leave it to the user to put a rng(foo); in front of the block in which reproducibility is required?

FFroehlich commented 7 years ago

that is indeed true, if you do not know about this option (and it's default), it can be very confusing if the user does not know about it being used. So its probably better do set the random seed to [] by default. which means that the user needs to set options.rng = 0, which is effectively more text than rng(0).

JanHasenauer commented 7 years ago

Okay, to resolve the problem I would suggest to leave it out. I think however that it would be helpful to mention in the documentation that "reproducibility" is achieved by "rng(0)".

dweindl commented 7 years ago

Note: PestoSamplingOptions.RndSeed wasn't used anywhere anyways.