Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

probe options and object options now include relevant Fresnel spectrum propagation multislice parameters and are used in the fwd and adj operators #331

Merged
merged 3 commits into from
Aug 10, 2024

Conversation

a4894z
Copy link
Contributor

@a4894z a4894z commented Jul 31, 2024

Purpose

Previously the multislice class and resulting forward and adjoint operators didn't use the specified parameters (e.g. probe wavelength, probe field of view (FOV), and multislice propagation distance), and instead always used the defaults.

Now, these parameters are explicitly passed into the multislice class.

Also, I now use the same propagator that fold_slice uses, not the one without phase prefactors and with the square root taylor expansion.

Approach

The probe wavelength and probe field of view (FOV) are now part of probe_options and multislice propagation distance is part of object options.

These parameters are then used in the multislice class for a composite operator which uses fresnel spectrum propagation of the type fold_slice uses.

Pre-Merge Checklists

Submitter

  • Write a helpfully descriptive pull request title.
  • Organize changes into logically grouped commits with descriptive commit messages.
  • Document all new functions.
  • Click 'details' on the readthedocs check to view the updated docs.
  • Write tests for new functions or explain why they are not needed.
  • Address any complaints from pep8speaks.

Reviewer

  • Actually read all of the code.
  • Run the new code yourself; the included tests should make this easy.
  • Write a summary of the changes as you understand them.
  • Thank the submitter.

Ashish Tripathi added 3 commits July 31, 2024 13:29
@pep8speaks
Copy link

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

Line 41:50: E201 whitespace after '('
Line 41:61: E202 whitespace before ')'
Line 120:1: W293 blank line contains whitespace
Line 123:18: E201 whitespace after '('
Line 123:42: E201 whitespace after '('
Line 123:44: E201 whitespace after '('
Line 123:56: E202 whitespace before ')'
Line 123:61: E201 whitespace after '('
Line 123:76: E202 whitespace before ')'
Line 123:81: E501 line too long (99 > 80 characters)
Line 123:83: E251 unexpected spaces around keyword / parameter equals
Line 123:85: E251 unexpected spaces around keyword / parameter equals
Line 123:90: E202 whitespace before ')'
Line 124:18: E201 whitespace after '('
Line 124:42: E201 whitespace after '('
Line 124:44: E201 whitespace after '('
Line 124:56: E202 whitespace before ')'
Line 124:61: E201 whitespace after '('
Line 124:76: E202 whitespace before ')'
Line 124:81: E501 line too long (99 > 80 characters)
Line 124:83: E251 unexpected spaces around keyword / parameter equals
Line 124:85: E251 unexpected spaces around keyword / parameter equals
Line 124:90: E202 whitespace before ')'
Line 126:56: E201 whitespace after '['
Line 126:58: E202 whitespace before ']'
Line 127:56: E201 whitespace after '['
Line 127:58: E202 whitespace before ']'
Line 131:51: E201 whitespace after '('
Line 131:81: E201 whitespace after '('
Line 131:81: E501 line too long (140 > 80 characters)
Line 131:83: E201 whitespace after '('
Line 131:111: E202 whitespace before ')'
Line 131:138: E202 whitespace before ')'
Line 133:62: E201 whitespace after '('
Line 133:81: E501 line too long (140 > 80 characters)
Line 133:84: E201 whitespace after '('
Line 133:112: E202 whitespace before ')'
Line 133:121: E251 unexpected spaces around keyword / parameter equals
Line 133:123: E251 unexpected spaces around keyword / parameter equals
Line 133:139: E202 whitespace before ')'
Line 139:40: W291 trailing whitespace
Line 140:81: E501 line too long (93 > 80 characters)
Line 141:81: E501 line too long (81 > 80 characters)
Line 146:81: E501 line too long (108 > 80 characters)
Line 148:81: E501 line too long (90 > 80 characters)
Line 149:81: E501 line too long (90 > 80 characters)
Line 151:64: W291 trailing whitespace
Line 160:81: E501 line too long (120 > 80 characters)
Line 162:81: E501 line too long (121 > 80 characters)
Line 169:5: E116 unexpected indentation (comment)
Line 169:5: E303 too many blank lines (2)
Line 170:5: E116 unexpected indentation (comment)
Line 171:5: E116 unexpected indentation (comment)
Line 172:5: E116 unexpected indentation (comment)
Line 173:5: E116 unexpected indentation (comment)
Line 174:5: E116 unexpected indentation (comment)
Line 175:5: E116 unexpected indentation (comment)
Line 176:5: E116 unexpected indentation (comment)
Line 177:5: E116 unexpected indentation (comment)
Line 178:5: E116 unexpected indentation (comment)
Line 180:5: E116 unexpected indentation (comment)
Line 181:5: E116 unexpected indentation (comment)
Line 182:5: E116 unexpected indentation (comment)
Line 184:5: E116 unexpected indentation (comment)
Line 185:5: E116 unexpected indentation (comment)
Line 186:5: E116 unexpected indentation (comment)
Line 187:5: E116 unexpected indentation (comment)
Line 189:5: E116 unexpected indentation (comment)
Line 192:1: W391 blank line at end of file

Line 43:17: E251 unexpected spaces around keyword / parameter equals
Line 43:19: E251 unexpected spaces around keyword / parameter equals
Line 59:1: W293 blank line contains whitespace

Line 104:1: W293 blank line contains whitespace

Line 76:1: W293 blank line contains whitespace
Line 77:52: W291 trailing whitespace
Line 78:66: W291 trailing whitespace
Line 82:1: W293 blank line contains whitespace
Line 93:81: E501 line too long (81 > 80 characters)
Line 127:81: E501 line too long (81 > 80 characters)
Line 148:81: E501 line too long (81 > 80 characters)
Line 184:81: E501 line too long (81 > 80 characters)

Line 76:58: W291 trailing whitespace
Line 80:54: E201 whitespace after '('
Line 80:69: E202 whitespace before ')'
Line 82:65: W291 trailing whitespace

Line 355:81: E501 line too long (102 > 80 characters)

Line 42:29: E251 unexpected spaces around keyword / parameter equals
Line 42:31: E251 unexpected spaces around keyword / parameter equals
Line 43:30: E251 unexpected spaces around keyword / parameter equals
Line 43:32: E251 unexpected spaces around keyword / parameter equals
Line 47:44: E251 unexpected spaces around keyword / parameter equals
Line 47:46: E251 unexpected spaces around keyword / parameter equals

@a4894z a4894z requested review from stevehenke and a team July 31, 2024 19:23
Comment on lines -59 to +61
self.pixel_size,
self.probe_FOV,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you have replaced pixel_size with probe_FOV in all cases, so you can completely remove it from the API.

Comment on lines 42 to 49
self.propagation = propagation(
detector_shape=detector_shape,
norm = norm,
probe_shape=probe_shape,
wavelength=probe_wavelength,
probe_FOV=probe_FOV_lengths,
distance=multislice_propagation_distance,
**kwargs,
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably don't need to explicitly pass all of these variables down the stack because keyword arguments are implicitly passed to the component classes using **kwargs.

Comment on lines 252 to 255
probe_photons=self.probe_photons,
probe_wavelength=self.probe_wavelength,
probe_FOV_lengths=self.probe_FOV_lengths,
force_orthogonality=self.force_orthogonality,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use caution when handling unit sizes in the resample functions. I think this might not be correct because when you resample the grid, the real unit sizes of one pixel will change.

@a4894z a4894z merged commit 3def223 into AdvancedPhotonSource:main Aug 10, 2024
2 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants