Generate Gaussian Smooth data labels via App-wide data labeler

[duytnguyendtn]

Description

This is a quick PR to fix a (potential?) bug where the first smoothed dataset generated by the Gaussian Smooth plugin is missing the dataset name. As requested by Cami, all smoothed datasets should have consistent naming by having the original dataset represented in its name. I replaced the custom labeling logic with Jesse's app-wide data label generator for consistency.

Before:

After:

This is due to some weird (potentially buggy) behavior where the dataset field is unavailable until multiple "smoothable" data products are available:

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? Set this to bugfix milestone if this is a bug fix and needs to be released ASAP; otherwise, set this to the next major release milestone.
• After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

[kecnry]

what benefit does passing this through return_data_label provide?

[duytnguyendtn]

In this Gaussian Smooth case, not much. This is because Gaussian Smooth allows the user to overwrite the result. By putting check_unique=False, it effectively just returns the string.

The main reason I decided to pass it through anyways is mainly for consistency. Though this is the case with Gaussian Smooth, it may not be the case with other plugins. We should probably centralize our data label generation, especially if we want automatic uniqueness checks. So I pass it through because (1) I don't think it will cause any harm and (2) it can serve as a reminder for future development to follow this paradigm.

I can also just set the string manually like before if we don't want this

[kecnry]

@javerbukh was the return_data_label intended to be used for plugin labels? If so, I'd agree it makes sense to start introducing this, but otherwise I'm not sure its worth the overhead since plugin labels have their own logic to check for overwriting, etc.

[duytnguyendtn]

For more context, I originally thought I could provide the smoothing details (e.g. spatial-smoothed stddev 1.0) as the ext argument, but because we allow overwriting (i.e. don't enforce uniqueness), we can't; ext is only used if uniqueness is required

[javerbukh]

The first pass implementation did not include plugin labels because I believe there is already a mixin in utils that handles that (AddResult maybe?). I agree with @duytnguyendtn that I do not see the harm in doing it this way, especially since we may want to centralize all data label generation in the future, but I also see @kecnry point about unnecessary overhead (though if we're not checking for uniqueness it should always be a fast operation). So I don't think I can weigh in too heavily on one side or the other here.

[kecnry]

I also think this probably should have a changelog entry since it is a change in behavior. (And it wasn't really a bug - this was originally by design and I think we switched individual plugins later by request... are there any other that need updates to be consistent?)

[pllim]

I agree with Kyle that change log is necessary here.

I am pretty neutral on the new naming convention. I think POs will be the picky ones here.

[pllim]

Current CI failure is unrelated, though I do wonder if any tests would fail due to this change here.

[camipacifici]

Does what I asked :) thank you!

[pllim]

I restarted the CI 🤞

[codecov]

Codecov Report

Base: 91.93% // Head: 91.94% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (33f577a) compared to base (8f8106b).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1973   +/-   ##
=======================================
  Coverage   91.93%   91.94%           
=======================================
  Files         140      140           
  Lines       15310    15312    +2     
=======================================
+ Hits        14076    14079    +3     
+ Misses       1234     1233    -1     

Impacted Files	Coverage Δ
...default/plugins/gaussian_smooth/gaussian_smooth.py	97.72% <100.00%> (-0.06%)	⬇️
...gins/gaussian_smooth/tests/test_gaussian_smooth.py	100.00% <100.00%> (ø)
...daviz/configs/imviz/tests/test_astrowidgets_api.py	98.58% <0.00%> (+<0.01%)	⬆️
jdaviz/core/freezable_state.py	91.66% <0.00%> (+1.38%)	⬆️

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.

[kecnry]

I would slightly vote to either remove the app.return_data_label call or use it consistently across all plugins, but that doesn't need to block approval/merge if anyone disagrees and would prefer we just slowly transition to using that as we come across them.