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

Enhanced tpf.interact(): return pixel selection optionally. #1402

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

Conversation

orionlee
Copy link
Collaborator

@orionlee orionlee commented Jan 30, 2024

Close #1386

it requires a slight public API change for tpf.interact() (a new optional parameter and the correspond return object).

TODOs:

  • make the API more generic to leave room for future additions: return a dict object that holds the selected pixels mask, rather than the mask directly.
  • correspond tests
  • docstring update

Misc. changes from review:

  • rename return_selection_mask to return_selected_mask.

Changelog:

- Added ``return_selection_mask`` parameter to ``tpf.interact()``, to optionally 
  return the pixel selection as an aperture mask. [#1402]

@orionlee orionlee force-pushed the tpf_interact_return_selection_mask_1386 branch from 6d1141b to eceadbb Compare February 19, 2024 19:34
@orionlee
Copy link
Collaborator Author

The PR is pending feedback on the API changes. @keatonb , thoughts?

  • it adds an optional return_result parameter to `tpf.interact()
  • if return_result is True, it returns a dict . The key "selection_mask" points to the selected pixels mask.

See the example in #1386 (comment)

@keatonb
Copy link
Contributor

keatonb commented Feb 19, 2024

I think this is a very nice idea! I'm not a core lightkurve developer, so I don't have feedback to provide on API decisions. It's not intuitive to me that return_result would mean aperture mask. What other attributes does the return_result have? The corresponding light curve?

@orionlee
Copy link
Collaborator Author

It's not intuitive to me that return_result would mean aperture mask.

keatonb, thanks for the feedback.

return_result is indeed cryptic, and would require docstring and examples to mitigate it if this path is chosen. Currently, it returns a dict with only 1 item "selection_mask".

This is done to leave room say, if we want to return additional data without changing the API again, conceivably, the x / y limits (that an user zooms), the single cadence / time the user taps.

The alternative would make the API change specific to the mask, e.g., a parameter return_selected_mask that returns the mask ndarray.

@Nschanche
Copy link
Collaborator

I agree the dictionary could make sense as it may be easier add additional parameters down the line, but I think returning an ndarray makes more sense in lightkurve. I would lean toward a parameter defaulting to false return_mask=False.

@orionlee orionlee force-pushed the tpf_interact_return_selection_mask_1386 branch from eceadbb to 2fafc0f Compare February 26, 2024 19:17
@orionlee
Copy link
Collaborator Author

The API is reverted back to directly returning the mask array.

Docstring and tests are added. The PR is ready to be merged, pending any further feedback.

Example:

interact_mask = tpf.interact(return_selection_mask=True);

# make a copy of the selection to avoid accidental pixel selection changes in the interact.
my_custom_mask = interact_mask.copy()

Copy link
Collaborator

@Nschanche Nschanche left a comment

Choose a reason for hiding this comment

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

Great added functionality!

>>> interact_mask = tpf.interact(return_selection_mask=True) # doctest: +SKIP
>>> # Once the desired pixels have been selected, save the result in
>>> # a separate variable to "freeze" the selection.
>>> my_custom_mask = interact_mask.copy() # doctest: +SKIP
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this need to be copied to edit interact_mask? I don't think that is the case, so I might skip the my_custom_mask example in the header.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

interact_mask.copy() is not strictly necessary, but is often needed in practice. That's why I include it in the example.

Reason:
interact_mask reflects the live / active selection. In practice, the selection can easily be inadvertently changed, surprising users. Scenario:

  1. run interact() in a cell, and select a custom aperture
  2. use the custom aperture (in interact_mask) to create a lightcurve.
  3. do something else in other cells.
  4. try to use the custom aperture again.

In my experience, after step 3 (do something else in other cells), in many cases the selection in interact() would be changed (to no pixel selected usually) [*]. In such scenario, if users do not pay attention and use the live interact_mask, they would have used a different aperture from what they initially selected.

The step of my_custom_mask = interact_mask.copy() in the example is one way to "freeze" / save the custom aperture mask while the users are still conscious of what they have just selected, avoiding any surprise from inadvertent changes.


[*] After I use tpf.interact(), scroll to other parts of the notebook, and scroll back to tpf.interact() cell, in many cases the selection is somehow lost:

image

@@ -1000,6 +1000,7 @@ def show_interact_widget(
vmax=None,
scale="log",
cmap="Viridis256",
return_selection_mask=False,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think return_selected_mask is more apt (rather than selection) as it is actively selected when using interact.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could rename return_selection_mask to return_selected_mask.
I'll wait for any other suggested changes from review feedback, and update the codes in 1 batch.

tests/test_interact.py Show resolved Hide resolved
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.

Enhance TargetPixelFile.interact() to let user access the selected pixels mask directly in Jupyter notebooks
3 participants