Closed briandleahy closed 3 years ago
Hello @briandleahy! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
holopy/scattering/tests/test_imageformation.py
:Line 201:22: E203 whitespace before ',' Line 207:22: E203 whitespace before ',' Line 207:34: E203 whitespace before ',' Line 208:22: E203 whitespace before ',' Line 209:46: E203 whitespace before ',' Line 210:58: E203 whitespace before ',' Line 211:70: E202 whitespace before ']' Line 213:22: E203 whitespace before ',' Line 214:22: E203 whitespace before ',' Line 215:22: E203 whitespace before ',' Line 216:22: E203 whitespace before ',' Line 216:46: E203 whitespace before ',' Line 217:22: E203 whitespace before ',' Line 231:15: E203 whitespace before ',' Line 231:20: E203 whitespace before ',' Line 231:25: E203 whitespace before ',' Line 231:30: E203 whitespace before ',' Line 231:35: E203 whitespace before ',' Line 234:15: E203 whitespace before ',' Line 234:40: E203 whitespace before ',' Line 234:65: E203 whitespace before ',' Line 235:30: E203 whitespace before ',' Line 235:55: E203 whitespace before ',' Line 273:38: E203 whitespace before ',' Line 273:52: E202 whitespace before ']' Line 275:52: E202 whitespace before ']' Line 276:38: E203 whitespace before ','
Thanks for doing this! The new organization is a big improvement, and the code and docstrings are generally much clearer. Most of my comments aren't specific fixes, but more things to consider/start a discussion.
Superseded by #379
This PR composes out a lot of the work of the old
ScatteringTheory
class into two new classes, a (much lighter)ScatteringTheory
and a newImageFormation
class.Rationale Most of the old
ScatteringTheory
code was actually for calculating images and not for scattering calculations per se. This made creating and modifying new scattering theories unwieldy. In particular, it was difficult to know which methods needed to be implemented to create a new scattering theory. This is now fixed. Moreover, while most of the time in a hologram calculation (say) was spent in the oldScatteringTheory
class, that time is mostly in dealing with xarrays and not in doing scattering calculations per se. Now that these two responsibilities are separated, performance optimizations will be easier to implement.Code Changes The code that was previously in
holopy.scattering.theory.scatteringtheory
is now split betweenholopy.scattering.theory.scatteringtheory
andholopy.scattering.imageformation
. In addition, there were some minor changes to the individual scattering theories, particularly theLens
theory, to work with the new changes. (TheLens
theory relied on methods which were moved to theImageFormation
class; now it only relies onScatteringTheory
methods.)API Changes All of these changes are internal and not visible to outside users.