search

manoharan-lab / holopy

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

Composescatteringtheory keepblame #379

Closed briandleahy closed 3 years ago

briandleahy commented 3 years ago

The same as #377, but modified to keep the git blame for each line of code despite the split of scatteringtheory into two files. I've kept the commit hashes exactly the same.

Since this PR should be the one that gets merged in, I've copied the notes from #377 below.

This PR composes out a lot of the work of the old ScatteringTheory class into two new classes, a (much lighter) ScatteringTheory and a new ImageFormation 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 old ScatteringTheory 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 between holopy.scattering.theory.scatteringtheory and holopy.scattering.imageformation. In addition, there were some minor changes to the individual scattering theories, particularly the Lens theory, to work with the new changes. (The Lens theory relied on methods which were moved to the ImageFormation class; now it only relies on ScatteringTheory methods.)

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 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 ','

Comment last updated at 2020-12-14 21:03:13 UTC
briandleahy commented 3 years ago

git blame is correct on this PR. Closing the other two.

briandleahy commented 3 years ago

There is also no code difference between this and #377, as evidenced by a git diff 7cf2e484 21287449

barkls commented 3 years ago

Nice work getting the blame history to follow!

I hate to be a pain, but can you update the releasenotes file even though the changes aren't user-facing? Sorry I didn't think of it earlier...

Also see my minor note on #377.

briandleahy commented 3 years ago

The release notes should be updated in a5c62fb9