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

Return peak stats in focus_from_transverse_band #179

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ieivanov
Copy link
Contributor

@ieivanov ieivanov commented Dec 5, 2024

On the mantis microscope, I find it useful to return some statistics about the detected peak, even if it doesn't pass the FWHM threshold. This allows me then to extend the search range during O3 autofocus.

This is a breaking change since we are returning multiple arguments. Other software would need to be updated as:

focus_index = focus_from_transverse_band[0]

An alternative implementation would be to have a optional function argument return_peak_stats=False and a statement like

if return_peak_stats:
    return in_focus_index, peak_stats
else:
    return in_focus_index

but I think that's not ideal from a programming standpoint. Leaving this in a branch is not a blocker for me, but it would be nice to see it merged with the next major release of waveorder.

@talonchandler
Copy link
Collaborator

I 100% agree that we need these features, but I'm hesitant to make breaking changes here. I also think the focus_from_transverse_band[0] syntax is a bit confusing.

I think there's an opportunity for a smoother refactor. What do you think about the following:

  • Leave focus_from_transverse_band(zyx_array) -> in_focus_index as is.
  • Factor out a new transverse_band_power(zyx_array) -> z_array_of_transverse_power function from focus_from_transverse_band. This is inspired in part by your suggestion, @edyoshikun's needs, and my needs---I think all three of us have needed something like this.
  • Add a new focus_statistics(z_array_of_transverse_power) -> Dict function that returns as many statistics as you need.

@ieivanov
Copy link
Contributor Author

I also think the focus_from_transverse_band[0] syntax is a bit confusing.

The alternative would be to use focus_index, _ = focus_from_transverse_band(zyx_array), which I agree looks a little cleaner.

Happy if we can refactor this further. If we make a new focus_statistics(z_array_of_transverse_power) function how would we get the focus index - would that be one of the keys in the dict it returns?

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.

2 participants