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

API: Move finite support probe constraints to probe constraints section #326

Merged
merged 2 commits into from
Jul 24, 2024

Conversation

carterbox
Copy link
Contributor

Purpose

Organize the code and make algorithm implementation less complicated by moving the probe constraints all to one section.

Approach

Convert the additional probe penalty and finite support probe constraints to the probe constraint section, and implement them as soft constraints.

Adjust the constrain center peak function to only take single pixel steps. This prevents large and sudden movements of the probe function in order to keep it centered. The method for determining the center is now also center of mass instead of finding the brightest spot.

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.

@pep8speaks
Copy link

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

Line 180:1: E302 expected 2 blank lines, found 1
Line 184:8: E231 missing whitespace after ','
Line 184:10: E231 missing whitespace after ','

@carterbox carterbox requested a review from a4894z July 22, 2024 16:25
Copy link
Contributor

@a4894z a4894z left a comment

Choose a reason for hiding this comment

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

Looks good, I just had one main question/clarification for the probe.py file

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the main change for centering the probe is using the center of mass of the probe intensity instead of its max value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's the main change. However, there are also a few implementation details which I think I mentioned in the commit message?

  1. The gaussian filter used to smooth the probe beforehand is now truncated at 6 sigma.
  2. The gaussian filter now assumes there is no probe outside the probe array (cval=0.0)
  3. The sigma is chosen so 6 sigma is the probe width.
  4. The shifts applied to the probe are now only elements from the set [-1, 0, 1]. This keeps the shifts small and avoids interpolation error because all shifts are integer.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like this way better with all the probe constraints applied here instead of in the rPIE or LSTSQ code.

Also, at some point I think we can still use Difference Map ideas for applying these probe or sample constraints...I'll show you what I mean when we meet tomorrow afternoon.

@a4894z a4894z self-requested a review July 24, 2024 14:56
@a4894z a4894z merged commit ccd45b9 into AdvancedPhotonSource:main Jul 24, 2024
3 of 7 checks passed
@carterbox carterbox deleted the probe-updates branch July 24, 2024 20:57
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