pep8speaks
commented
4 years ago Hello @barkls! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
- In the file
holopy/fitting.py:
Line 55:5: E128 continuation line under-indented for visual indent
- In the file
holopy/inference/model.py:
Line 50:25: E127 continuation line over-indented for visual indent
- In the file
holopy/inference/tests/test_model.py:
Line 175:80: E501 line too long (80 > 79 characters) Line 176:80: E501 line too long (80 > 79 characters)
briandleahy
barkls
Fixes #353. Fixes #256.
Moved all parameter handling from Scatterer class to Model class. This fixed some problems with tied parameters and model flexibility. The Model now stores scatterer, alpha, optics, and hopefully theory in the form of a "map" that tells it how to reconstruct the object with appropriate parameter substitutions. This PR largely rewrites the model.py file and that is the most important place to look to understand what's going on. Everything else is mostly just adapting to those changes.
There are a few scatterer API changes that I want to mention: 1) scatterer.parameters used to give a flat dictionary, e.g.
{n.real: Uniform(1.58, 1.60), n.imag: 0.001, 'center.0': 10, 'center.1':10, 'center.2':Uniform(0, 100)}. After this PR it matches the initial arguments to call the scatterer:{'n': ComplexPrior(Uniform(1.58, 1.60), 0.001), 'center': (10, 10, Uniform(0, 100))}. 2)scatterer.from_parametersused to always give a definite value - it would substitute all priors with their best guess. With this PR it just returns priors without modification. 3) Previously you could callcalc_holoon a parameterized scatterer and it would similarly substitute priors with their best guess. This PR requires you to define a model object if you want to do that (but you were going to define a model anyway if using a parameterized scatterer).There's also a big model API change that I'm less sure about and would appreciate some feedback on: Previously, model methods (lnprior`, lnlike, residuals, lnposterior, forward, and the new scatterer_from_parameters) wanted a dictionary of parameter values to evaluate at, e.g. {'r': 0.5, 'n': 1.59, 'alpha': 0.8}. With the changes in this PR, I found a list of parameter values was more natural, and there was a lot of switching back and forth between them if we wanted to keep dictionaries everywhere. After this PR, the methods expect a list of parameter values in a specific order e.g. [0.5, 1.59, 0.8]. They can still take a dictionary for backwards compatibility but it is deprecated.
Unfortunately both formats have advantages, and this is where I'd like input. The dictionary is more human readable and lets you easily test how different parameter values would affect the hologram, for example. It matches
model.parameterswhich is the main user-facing way to see what the model parameters are. A list makes for cleaner code, and a simpler interface for inference algorithms, both within holopy an externally. This corresponds tomodel._parameters, which does all the work but isn't user facing.