Closed barkls closed 3 years ago
Hello @barkls! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:
That won't work because sometimes priors are sequestered in other objects - e.g. a list of center coordinates or a list of refractive indices for a multilayer sphere. Or for multicolour, a dictionary or xarray of refractive indices (or a list of dictionaries if multilayer and multicolour), etc. That's the purpose of converting paramters to a mapping in the Model class. This code previously belonged to scatterers in a different format, but then we had problems where the model couldn't easily use it for things like alpha or ScatteringTheory parameters so now it belongs to the Model. My previous solution (before this PR) was to just say "Users need to wrap their parameterized scatterer in a Model() if they want to use calc_holo", but now I've tried to just do it for them with this PR.
Thanks, that's a good solution! Metaclasses are a part of Python I wish I understood better so this will be a good learning exercise.
On Fri., May 21, 2021, 11:01 Brian Leahy, @.***> wrote:
@.**** commented on this pull request.
In holopy/inference/prior.py https://github.com/manoharan-lab/holopy/pull/400#discussion_r636990028:
@@ -412,6 +412,11 @@ def map_keys(self): return ((key, getattr(self, key)) for key in ['real', 'imag'])
+# Any new prior types should be added to: +# holopy.scattering.interface.PRIOR_TYPES +# holopy.scattering.tests.test_interface.TestValidateScatterer.priors + +
we also need an example instance of each prior class to test, and right now there's no way to generate that.
I think that TestValidateScatterer.test_all_priors_accounted_for will solves this problem without the need for generating an example instance, as long as the Prior metaclass adds the class name to holopy.scattering.interface.PRIOR_TYPES. Then TestValidateScatterer.test_all_priors_accounted_for will raise an error if a prior type is in holopy.scattering.interface.PRIOR_TYPES and not in TestValidateScatter.priors. (Perhaps TestValidateScatterer.priors could be a dict or something to ensure that each class is represented, rather than just the lengths being the same. Likewise, perhaps the test gives a more descriptive error message to the developer if it fails.) I think adding to the tests is part of TDD, so the future developer should expect to do that anyways :)
Again, I think this may be the "right" way to do it, but I'm not sure if it's worth it given that there are only 4 prior types and I don't foresee a future proliferation of prior types in holopy. It's up to you.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/manoharan-lab/holopy/pull/400#discussion_r636990028, or unsubscribe https://github.com/notifications/unsubscribe-auth/AETUFJZGWNWF677DLTDWXT3TOZYTBANCNFSM45HKRXDA .
Actually we just need the map interpretation to handle parameterized scatterers, not the full Model class. I should be able to break that out into a different file (maybe prior.py) to resolve the circular dependency.
Hmm....
Allow me to just spitball some thoughts, and tell me what you think.
I think the source of the problem is that the Model
class does too many things: It both knows how to calculate a model, and it knows how to create scatterers, theories, etc from priors. I think that ideally the latter portion should be split off into a separate object (perhaps an InferenceMap
class) that resides in a separate sub-module (e.g. holopy.inference.maps
). Then both holopy.scattering.interface
and holopy.inference.model
could import this separate module without creating a circular dependency graph.
A more drastic alternative would be to move all the user-facing stuff (everything in holopy.inference.interface
and holopy.scattering.interface
) to a separate sub-model (e.g. holopy.interface
), which would also break the circular dependency and make it clearer what code is back-end and what code is front-end.
That being said
Perhaps what we can do is
Thoughts? If you think this is reasonable, I'll go ahead and merge this in when you're ready (just let me know).
You're right, I forgot I tried previously to extract all the mapping stuff from Model and was unsuccessful. I think we don't have a solution we are confident about that is also within the scope of how much work we are trying to do here, so I will leave it as is for now and put a comment in the code about what's going on.
I'll make changes based on your other comments and let you know when it's ready to merge.
On Fri., May 21, 2021, 11:17 Brian Leahy, @.***> wrote:
@.**** approved this pull request.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/manoharan-lab/holopy/pull/400#pullrequestreview-665580200, or unsubscribe https://github.com/notifications/unsubscribe-auth/AETUFJ3XVWILEJNXXRF4JI3TOZ2SDANCNFSM45HKRXDA .
Ok a few notes:
However, splitting out parameter mapping didn't resolve circular dependencies.
We have
inference.model.py
->scattering.interface.py
->inference.parameter_mapping.py
->inference.prior.py
-> Nothing
with arrows indicating dependency.
This looks like it's ok, except that there's a secret inference.__init__.py
hidden between scattering.inferface.py
and inference.parameter_mapping.py
, and the init depends on inference.model.py
so there's still a circular dependency. I didn't know the __init__.py
would interfere like this and from googling it doesn't look like there's any way around it.
parameter_mapping.py
and prior.py
from inference to core. This seems like a major change to architecture but I import prior back into the inference.__init__.py
so you can still from holopy.inference import prior
but you can no longer from holopy.inference.prior import Uniform
. We have nearly always used the first version in docs and examples so this shouldn't break anything. This change also lets priors and parameter mapping be more widely accessible within the HoloPy package. However, I'm not convinced it's a good idea since they do seem like inference-related concepts. What do you think?paramter_mapping.py
and prior.py
back in inference, at the cost of moving the import parameter_mapping
of scattering.interface.py
back into a function and preventing easy use of priors and parameter mapping outside of the inference module.Wow a lot of changes @barkls ! I hope my review didn't come across as too demanding.... If so I apologize. But overall these changes look awesome and I'm happy to take a look at them..
To reply in random order:
- There are a bunch of small unrelated changes. Commit messages should be helpful for understanding them.
Yep, I saw them, and the small bugfixes look good. Thanks!
- However, splitting out parameter mapping didn't resolve circular dependencies.
- I did get around this problem by moving parameter_mapping.py and prior.py from inference to core. This seems like a major change to architecture but I import prior back into the inference.init.py so you can still from holopy.inference import prior but you can no longer from holopy.inference.prior import Uniform. We have nearly always used the first version in docs and examples so this shouldn't break anything. This change also lets priors and parameter mapping be more widely accessible within the HoloPy package. However, I'm not convinced it's a good idea since they do seem like inference-related concepts. What do you think?
- We can revert that commit, putting paramter_mapping.py and prior.py back in inference, at the cost of moving the import parameter_mapping of scattering.interface.py back into a function and preventing easy use of priors and parameter mapping outside of the inference module.
I think this is a good solution to the circular dependency problem for now. :+1:.
- The changes after your feedback are as big (or bigger) than the original PR. I know this is annoying to review. Sorry.
no problem ;)
- I did manage to split the parameter mapping out from the model. It wasn't as hard as I thought and things are much cleaner now. Yay!
- As a side effect, this removed the need to make a prior metaclass, since the mapping takes care of it.
Awesome. Awesome. I just have a few minor comments and one dumb question about the new mapper:
Mapper
is in holopy.core.parameter_mapping
, but it looks like the tests are in holopy.inference.tests.TestParameterMapping
. Do we want to move them to core.tests
?holopy.core
is a fine place for the Mapper
, the location does mean that it's a bit confusing to figure out what it does (since core
and Mapper
are both generic names). I think either a more descriptive name (e.g. InferenceParameterMapper
), a 1-line docstring for the Mapper (e.g. """Tool for creating parameters to automatically pass in to holopy functions"""
), or both would help clarify what the purpose of the code is for future developers.mapping.py
), but this is just personal preference and you should trust yours instead.These are just suggestions. I'm happy to merge this in as is. Let me know when you want me to merge.
Thanks for the review! Your previous comments weren't demanding but I kept finding things to fix...
I followed all your suggestions. Please review one more time and merge in if everything looks good.
The purpose of the Mapper
class is to create maps (duh) but also to keep a running list of parameters (prior objects) and their corresponding names. This is important to be able to tie together parameters that are actually the same object and make sure all parameters have compatible names, even if they aren't mapped at the same time. The Model
class needs to create multiple maps at instantiation, and it uses a Mapper
object to keep ties and names consistent across all of them. After doing that once, the Model
won't need to do it again (although ties can still be adjusted with the Model.add_tie
method) so it has no more need for the Mapper
object. It steals the parameters
and parameter_names
attributes from the Mapper
and leaves all the methods behind without saving them. The Model
can unpack packs using separate functions in mapping.py
that don't need to store information across calls so aren't assocated with a class.
Let me know if that explanation makes sense!
Changes not pushed yet, hang on...
You should be happy with this PR - lots of red on code files, the only big green block is a test file!
The main thing I'm unhappy about is the Model import statement in the middle of
hp.scattering.interface.py
. Unfortunately I couldn't figure out way around it without splitting model.py into two files (one containing the baseModel
class, and one containingAlphaModel
andExactModel
). I don't think breaking the helper functions out ofhp.scattering.interface.py
is enough to solve the circular dependency. Let me know if I missed something with this or anything else!