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

gaussian smooth: expose spatial smoothing in API #1699

Merged
merged 5 commits into from
Oct 6, 2022

Conversation

kecnry
Copy link
Member

@kecnry kecnry commented Oct 4, 2022

Description

This pull request exposes spatial (in addition to spectral) smoothing in the user API for the gaussian smooth plugin, introduced in #1401, for cubeviz.

This PR renames the apply_spectral_smooth to spectral_smooth, and exposes two new methods:

  • apply_smooth(add_data=True) smooth(add_data=True) (which listens to the mode and the results label component)
  • spatial_smooth() (only exposed for cubeviz, just returns a spectrum1D object)
  • spectral_smooth() (just returns a spectrum1D object)

In doing so, this also renames the previous (non-public) apply_spatial_convolution to spatial_smooth.

I decided to expose the individual methods separately so that it isn't necessary to set all the plugin options for the mode if you just want to access the results in the plugin, but if anyone thinks that is too redundant, I'm also happy to hide spatial_smooth and spectral_smooth and instead require the user to set plugin.mode and then call plugin.apply_smooth(add_data=False).

Since #1401 has not been included in a release, this does not need to be considered breaking changes (as long as we get this in before the next release).

Updated plugin API docs

Change log entry

  • Is a change log needed? If yes, is it added to CHANGES.rst? If you want to avoid merge conflicts,
    list the proposed change log here for review and add to CHANGES.rst before merge. If no, maintainer
    should add a no-changelog-entry-needed label.

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

  • Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
  • Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
  • Do the proposed changes follow the STScI Style Guides?
  • Are tests added/updated as required? If so, do they follow the STScI Style Guides?
  • Are docs added/updated as required? If so, do they follow the STScI Style Guides?
  • Did the CI pass? If not, are the failures related?
  • Is a milestone set?
  • After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

@kecnry kecnry added cubeviz plugin Label for plugins common to multiple configurations labels Oct 4, 2022
@kecnry kecnry added this to the 2.11 milestone Oct 4, 2022
@kecnry kecnry force-pushed the api-spatial-smooth branch from f7776d4 to ced1443 Compare October 4, 2022 15:59
@kecnry kecnry marked this pull request as ready for review October 4, 2022 16:10
@kecnry kecnry requested a review from camipacifici October 4, 2022 16:10
@codecov
Copy link

codecov bot commented Oct 4, 2022

Codecov Report

Base: 87.17% // Head: 87.20% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (83a3e73) compared to base (5bcbb76).
Patch coverage: 96.66% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1699      +/-   ##
==========================================
+ Coverage   87.17%   87.20%   +0.02%     
==========================================
  Files          95       95              
  Lines        9936     9939       +3     
==========================================
+ Hits         8662     8667       +5     
+ Misses       1274     1272       -2     
Impacted Files Coverage Δ
...default/plugins/gaussian_smooth/gaussian_smooth.py 98.86% <96.66%> (+2.39%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@camipacifici
Copy link
Contributor

If I understand correctly

smoothing = viz.plugins['Gaussian Smooth']
smoothing.dataset = "whatever"
smoothing.mode = 'Spectral'
smoothing.stddev = 5
smoothing.apply_smooth(add_data=True)

and

smoothing = viz.plugins['Gaussian Smooth']
smoothing.dataset = "whatever"
smoothing.stddev = 5
smoothing.spectral_smooth()

do the same thing, but the first updates the entries in the GUI and shows the result, while the second is more compact but does not update the GUI or shows the result there.
My personal vote is to NOT expose smoothing.spectral_smooth and smoothing.spatial_smooth and give only the first option to the user. Happy to be convinced otherwise if people can come up with a good case.

@kecnry
Copy link
Member Author

kecnry commented Oct 5, 2022

Ok, it is easier to re-introduce later than to hide later, so I removed the spectral_smooth and spatial_smooth methods from the user API (while keeping the internal code refactor). I've also renamed apply_smooth to smooth to be more consistent with the collapse plugin's collapse method.

I'm also not sure I love the name of the mode option, but couldn't think of anything clearer that is still concise. The collapse plugin will probably also eventually have a spatial vs spectral switch, and we would probably want to use the same pattern there. The collapse plugin currently uses method for mean/max/sum/etc, so we shouldn't use that here for spatial vs spectral (although maybe we should consider renaming collapse.method to collapse.function to be more consistent with the plot_options.collapse_function).

So..... how would everyone feel about the following additional changes:

  • gaussian_smooth.mode -> gaussian_smooth.along_axis or gaussian_smooth.direction (and plan to do the same for collapse in the future) (EDIT: people generally preferred sticking with mode for now)
  • collapse.method -> collapse.function or collapse.collapse_function (EDIT: implemented in collapse plugin: rename "method" to "function" #1702)

@PatrickOgle
Copy link
Contributor

Can we instead make this implicit in the API:
smoothing.spectral.stddev = 5 invokes a spectral smoothing of 5
smoothing.spatial = 5 invokes a spatial smoothing of 5

@PatrickOgle
Copy link
Contributor

I am not sure collapse.function is any less ambiguous than collapse.method
Furthermore, both are python terms that could be confusing. I can't think of a good alternative at the moment, though...

@pllim
Copy link
Contributor

pllim commented Oct 5, 2022

Code wise, LGTM, but looks like POs have concerns about the API, so I'll withhold technical review until that is settled.

@kecnry
Copy link
Member Author

kecnry commented Oct 6, 2022

@pllim - I think the API discussion is settled, at least for now (please anyone correct me if I'm wrong, but I think the general suggestion was to stick with mode here and to change method to function in #1702). We may have to iterate further in the future, but this is at least a step in the right direction that I think we should try to get in before the next release to avoid absolutely needing to deprecate the API currently in main.

kecnry added 5 commits October 6, 2022 10:03
the output label now exists and shows "overwrite" on the button
* to be more consistent with other plugins (especially collapse)
* and to have description change depending on whether spatial smoothing is available
@kecnry kecnry force-pushed the api-spatial-smooth branch from a0b7484 to 83a3e73 Compare October 6, 2022 14:03
Copy link
Contributor

@pllim pllim left a comment

Choose a reason for hiding this comment

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

Code-wise LGTM. Thanks!

Copy link
Contributor

@camipacifici camipacifici 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 from PO perspective.

@pllim pllim merged commit 6be7a44 into spacetelescope:main Oct 6, 2022
@kecnry kecnry deleted the api-spatial-smooth branch October 6, 2022 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cubeviz plugin Label for plugins common to multiple configurations Ready for final review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants