search

manoharan-lab / holopy

Hologram processing and light scattering in python
GNU General Public License v3.0
131 stars 50 forks source link

Fittablescatteringtheory #387

Closed briandleahy closed 3 years ago

briandleahy commented 3 years ago

Fixes #348 .

@barkls , before taking the time to do an in-depth code review, can you take a quick look at the overall changes I made and lmk if you think they are sufficient? Briefly

If so, I'll update the docs, both in "whats new" and the scattering tutorial.

pep8speaks commented 3 years ago

Hello @briandleahy! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 376:13: E741 ambiguous variable name 'l' Line 382:80: E501 line too long (80 > 79 characters) Line 393:17: E741 ambiguous variable name 'l'

Comment last updated at 2021-03-17 21:16:29 UTC
barkls commented 3 years ago

This looks good at first glance! The only thing I'm wondering about is whether there's redundancy in terms of defining a ScatteringTheory - it looks like any __init__ parameters need to either end up in parameter_names or unfittable attributes. Is that right? If these two tuples are mutually exclusive and exhaustive of all __init__ parameters you might be able to get rid of one of them by using the _dict property defined in holopy_object.py. You might even be able to simplify from_parameters to:

kwargs = self._dict
kwargs.update(parameters)
return self.__class__(**kwargs)

Untested code, might need a copy() in there to avoid modifying the theory.

Failing travis is due to a new xarray deprecation warning that we need to fix, but unrelated to this PR

briandleahy commented 3 years ago

Good call! The _dict is a collection of the init args based on their value in the current object, which is what I want. There is some possibility of this causing a problem if (i) someone writes a class where the init takes arguments that aren't set as attributes, or (ii) we move away from yaml and the _dict property gets moved, but since _dict is used in a few places throughout I don't think it will be a problem.

I'll add this in with some tests then make this a real PR.

barkls commented 3 years ago

One more thing I noticed but required a bit more thinking regards what is stored in model.theory. For scatterers, we don't actually store the scatterer - we store a dummy scatterer that maintains the correct from_parameters and other attributes like warn=True/False for Spheres. We also store instructions for how to convert raw parameters into something compatible with the dummy scatterer's from_parameters in the maps dictionary. This organization ensures that we can easily store the model and all of its attributes as yaml regardless of what the initial scatterer looked like.

Right now, you aren't doing something similar for the theory. This could cause problems for cases where the theory object can't be written to yaml, prevent saving of the model or inference result object to a file. I believe this would only happen if the theory contains xarrays, e.g. if you want to specify a different lens angle (fixed value or prior) for two different illuminations, as defined by wavelength and polarization. Does the scattering theory even support this use case? If the answer is no, we're all good. If the answer is yes, we should either explicitly forbid this edge case, or work around it.

briandleahy commented 3 years ago

A few thoughts:

  1. I don't think the ScatteringTheory supports an xarray use case.... (Your multicolor code is pretty powerful, so I'm not 100% sure.)
  2. The reason I create the theory is because the default string 'auto' obviously doesn't have a .from_parameters method. However, I could do something like what you did with the scatterer, like so: 
    # in the init
self._dummy_theory = interpret_theory(self._dummy_scatterer, theory)

    and

    def theory_from_parameters(self, pars):
    pars = self.ensure_parameters_are_listlike(pars)
    formatted = read_map(self._maps['theory'], pars)
    return self._dummy_theory.from_parameters(formatted)
  3. I'm a little confused why that works for the scatterer, since the _dict property should miss the scatterer then.....
barkls commented 3 years ago

I don't think the xarray use case would work either, but it might be worth trying it. You're right theory needs to be handled differently to support 'auto' case. The current scatterer implementation is pretty tricky! actually broken, which I rubber duck'ed to realization while writing out how it works. The Model.scatterer property recreates the original scatterer that is passed in, which causes problems when loading from yaml if it included xarrays. I can't remember if there was another reason to create the _dummy_scatterer but it's failing at this purpose...

All that to say, I don't think you need to worry about this edge case for the theories. Let me know if the PR is ready for a full review.

briandleahy commented 3 years ago

Yeah, I think this is part of a bigger problem with yaml + xarray, so I'll leave it out of this PR. Some preliminary work towards it is on my branch yamlfittablescatteringtheory, in 1591c1b2 on my fork. I'll leave it up for a bit if someone wants to take a look at it.

I think this is ready for an actual review, if you'd like. @ralex0 I'd also appreciate your eyes on it if you have the time. If you could let me know if there are fitting parameters / hyperparameters in the Lens class that I'm missing, that would be great.

briandleahy commented 3 years ago

Looks good. Won't merge with failing tests though, so you can pin xarray on travis or wait for a PR from me including that fix.

Pinned travis to <0.17, which seems to be the breaking change (released in Feb). LMK what your thoughts are on the deprecating of PerfectLensModel and I'm ready to merge.