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

Fix profile penumbra part deux #327

Closed

Conversation

randlet
Copy link
Contributor

@randlet randlet commented Nov 20, 2020

This is a replacement for PR #324 and includes a fix for LeedsTOR which broke due to improper peak detection.

Warning: I don't 100% understand the difference between threshold & x in find_fwxm_peaks and I need to investigate more whether it breaks anything else. Any thoughts?

This patch resolves issue jrkerns#323 by switching to detecting the
outermost points that cross the threshold value when detecting
field edges in SingleProfile.
Using default x=50 for find_fwxm_peaks in LeedsTOR caused the wrong peak to
be detected. (Don't understand 100% why :?)
@randlet
Copy link
Contributor Author

randlet commented Nov 20, 2020

Not sure why yet but this PR breaks the starshot demo.

@jrkerns
Copy link
Owner

jrkerns commented Nov 20, 2020

Let me know if these help:

Parameters
----------
x : int, float
The Full-Width-X-Maximum desired. E.g. 0.7 will return the FW70%M.

and
threshold : int, float
The value the peak must be above to be considered a peak. This removes "peaks"
that are in a low-value region.
If passed an int, the actual value is the threshold.
E.g. when passed 15, any peak less with a value <15 is removed.
If passed a float, it will threshold as a percent. Must be between 0 and 1.
E.g. when passed 0.4, any peak <40% of the maximum value will be removed.

@randlet
Copy link
Contributor Author

randlet commented Nov 20, 2020

I read those but somehow confused myself. Yes that makes sense. Thanks.

@randlet
Copy link
Contributor Author

randlet commented Nov 20, 2020

With this PR the two outside peaks don't get detected correctly.

starshot-peaks

@jrkerns
Copy link
Owner

jrkerns commented Nov 20, 2020

Not sure why yet but this PR breaks the starshot demo.

Looking at the starshot both with your recent change and this change now. I'm also doing some small refactoring.

@jrkerns
Copy link
Owner

jrkerns commented Nov 20, 2020

FYI the multiple image combine test has been failing for a while.

@jrkerns
Copy link
Owner

jrkerns commented Nov 20, 2020

Problem is somewhere in here:

original_peaks = copy.deepcopy(self.peaks)
for num, (peak, profile) in enumerate(zip(self.peaks, subprofiles)):
shift = original_peaks[num - 1].idx if num > 0 else 0
# shift = sum(len(profile.values) for profile in subprofiles[:num])
fwhmc = profile.fwxm_center(x, interpolate=interpolate)
peak.idx = fwhmc + shift

@randlet
Copy link
Contributor Author

randlet commented Nov 20, 2020

Just came to exact same conclusion! The first subprofile has this shape:

subprof

and the new penumbra point detection method is picking up the right hand side. Should the penumbra detection take the gradient of the profile into consideration? So when looking for a profile from RIGHT it should only consider points where gradient is negative?

@jrkerns
Copy link
Owner

jrkerns commented Nov 20, 2020

So the new code is finding the FWXM based on the indices found, and when you have a profile like this then it will use the furthest-out index. So the FWXM has a right side of 4800.
Figure_1

@randlet
Copy link
Contributor Author

randlet commented Nov 20, 2020

lol

@jrkerns
Copy link
Owner

jrkerns commented Nov 20, 2020

Just came to exact same conclusion! The first subprofile has this shape:

subprof

and the new penumbra point detection method is picking up the right hand side. Should the penumbra detection take the gradient of the profile into consideration? So when looking for a profile from RIGHT it should only consider points where gradient is negative?

I've not thought of using the sign of the gradient. That's pretty smart. I think this is why I did the center-looking-outward approach.

@jrkerns
Copy link
Owner

jrkerns commented Nov 20, 2020

In the find_peaks function there's a parameter search_region. This helps cut the left and right sides of the profile to avoid these cases. Unfortunately, I've not implemented this for FWXM methods.

@jrkerns
Copy link
Owner

jrkerns commented Nov 20, 2020

search_region : tuple of ints, floats, or both
The region within the profile to search. The tuple specifies the (left, right) edges to search.
This allows exclusion of edges from the search. If a value is an int, it is taken as is. If a float, must
be between 0 and 1 and is the ratio of the profile length. The left value must be less than the right.

@jrkerns
Copy link
Owner

jrkerns commented Nov 20, 2020

Hacky, but the smallest solution instead of passing through search_region everywhere is to cut off the left and right ends within subdivide to say, 80% width:

left_end = peaks[idx].idx
peak_idx = peaks[idx+1].idx - left_end
right_end = peaks[idx+2].idx

@randlet
Copy link
Contributor Author

randlet commented Nov 20, 2020

Check commit I just pushed

@@ -738,7 +738,7 @@ def _phantom_angle_calc(self) -> float:

start_angle_deg = self._determine_start_angle_for_circle_profile()
circle = self._circle_profile_for_phantom_angle(start_angle_deg)
peak_idx = circle.find_fwxm_peaks(threshold=0.6, max_number=1)[0]
peak_idx = circle.find_fwxm_peaks(threshold=0.6, x=80, max_number=1)[0]
Copy link
Owner

Choose a reason for hiding this comment

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

This x=80 is failing a number of tests. Why are you choosing 80?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No good reason...I think that I was somewhat confused and was experimenting with various parameters. (I'm not proud ☹️ )

@jrkerns
Copy link
Owner

jrkerns commented Nov 22, 2020

I think scipy copied me 😂: https://docs.scipy.org/doc/scipy/reference/generated/scipy.signal.find_peaks.html#scipy.signal.find_peaks

This is new in v1.1, which wasn't around when I created this. I'd much prefer to replace my implementation with theirs. @randlet are you okay with a scipy>=1.1 requirement?

@randlet
Copy link
Contributor Author

randlet commented Nov 22, 2020 via email

@jrkerns
Copy link
Owner

jrkerns commented Nov 22, 2020

Awesome. This is perfect. It will find peaks and also the penumbra points. Refactor....

@randlet
Copy link
Contributor Author

randlet commented Nov 22, 2020

Always nice when you can eliminate a heap of code :)

…ve threshold

Previous iteration of _penumbra_point detection failed if there
were multiple values above threshold in the profile.  This commit
ensures the point closest to the peak is used.
@randlet
Copy link
Contributor Author

randlet commented Nov 23, 2020

OK, I went back and actually figured out why the code wasn't working instead of sticking in an arbitrary x=80 🙄 . Thank you for calling me out on that!

The issue was the penumbra point detection still picked the wrong location if there were multiple places in the profile with the correct gradient and the correct threshold. I fixed it so that the location closest to the peak meeting those conditions is used and it all seems good now! This is maybe just academic at this point if you're factoring most of this code out now, but I needed a fix in the interim.

@jrkerns
Copy link
Owner

jrkerns commented Nov 24, 2020

I am refactoring it 🤷‍♂️. The scipy implementation is very good.

@randlet
Copy link
Contributor Author

randlet commented Nov 24, 2020

See if you can talk them into implementing phantom angle detection next 😆 !

@jrkerns
Copy link
Owner

jrkerns commented Nov 24, 2020

time to launch a Kaggle contest for it! 😂

@SimonBiggs
Copy link

See if you can talk them into implementing phantom angle detection next 😆 !

If there's interest in applying a similar logic for phantom angle detection, here is how I detect field image rotation for WLutz work:

https://github.com/pymedphys/pymedphys/blob/3738e899c9944bb592079ce017c04e07abd78226/lib/pymedphys/_wlutz/findfield.py#L197-L220

@SimonBiggs
Copy link

See if you can talk them into implementing phantom angle detection next laughing !

Probably a better and more clear example of using scipy's basinhopping to determine rotation:

https://github.com/pymedphys/pymedphys/blob/3738e899c9944bb592079ce017c04e07abd78226/lib/pymedphys/_electronfactors/core.py#L384-L422

@randlet
Copy link
Contributor Author

randlet commented Nov 24, 2020

Thanks! I'm implementing a PTW EPID QC phantom now so maybe I'll give that a shot.

@jrkerns jrkerns closed this Feb 9, 2021
jrkerns added a commit that referenced this pull request Jan 11, 2024
)

RAM-3200 Add refinement for 604 analyses

Approved-by: Randy Taylor
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.

None yet

3 participants