choderalab / pinot

Probabilistic Inference for NOvel Therapeutics
MIT License
15 stars 2 forks source link

rewrote net #32

Closed yuanqing-wang closed 4 years ago

karalets commented 4 years ago

@yuanqing-wang Let me add one more comment: OI think it would be good if you added a global variable self.noise_model_args which can be None typically, but could contain arguments that are relevant for arbitrary settings we may want to have.

I.e. when you have fixed variance per default we set it to 1.0. There may be cases where we may want to set it to some value based on normalization etc. Int hat case you could have noise_model_args.std = value and then use that inside the noise model if this is passed.

For more complex noise models one may want to add later it gives flexibility to add more details, as these args can be whatever they want to be and just inform the noise model.

What do you think?

yuanqing-wang commented 4 years ago

how about, leave the possibility open by allowing users to specify a dict about how they would like to handle distribution parameters and distribution class, namely

        else:
            assert isinstance(
                    self.noise_model,
                    dict)

            distribution = self.noise_model['distribution'](
                    self.noise_model['kwargs'])
yuanqing-wang commented 4 years ago

@karalets if you think the current implementation is ok I'll go ahead and rewrite many of the tests

karalets commented 4 years ago

how about, leave the possibility open by allowing users to specify a dict about how they would like to handle distribution parameters and distribution class, namely

        else:
            assert isinstance(
                    self.noise_model,
                    dict)

            distribution = self.noise_model['distribution'](
                    self.noise_model['kwargs'])

hmm i don't love this exact design, we can leave it for later

karalets commented 4 years ago

@karalets if you think the current implementation is ok I'll go ahead and rewrite many of the tests

yeah try to test this now, please