Cleanup plugin apis #3453

kecnry · 2025-02-19T19:20:24Z

Description

This pull request cleans up exposed plugin APIs which are meant for internal use only, including:

replacing **kwargs from subset_tools.import_region(..., **kwargs) with explicit region_format=None. If there are other kwargs passed to load_regions that we want the user to access, we should explicitly add and document those as well.
remove update=False in subset_tools.set_center(..., update=False) and hardcode to update=True. IMPORTANT: this is technically a change in behavior. Do we need to deprecate or is a changelog entry in a minor release sufficient? set_center was first made public in 4.1 by Rename Subsets Plugin #3304.. (see discussion at Cleanup plugin apis #3453 (comment))
remove models=None from model_fitting.get_model_parameters(models=None, ...). The helper-method is already deprecated and the plugin-method has not been released yet, so the existing deprecation should be sufficient.
remove models=None from model_fitting.get_models(models=None, ...). The helper-method is already deprecated and the plugin-method has not been released yet, so the existing deprecation should be sufficient.

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. Bugfix milestone also needs an accompanying backport label.
After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

* if we have need for any kwargs passed through load_regions, we can explicitly include/document those as well

* user API is now hardcoded to update=True, which was not the previous default (BEHAVIOR CHANGE)

* deprecation warning already exists for the old helper methods, and plugin APIs have not been released yet, so no need to add deprecations here

pllim · 2025-02-19T20:04:01Z

jdaviz/configs/default/plugins/subset_tools/subset_tools.py

@@ -830,6 +830,26 @@ def set_center(self, new_cen, subset_name=None, update=False):
            self.subset_definitions = []
            self.subset_definitions = tmp

+    def set_center(self, new_cen, subset_name=None):


Hmm I thought we agreed to just remove it from the public API exposure?

that's effectively what this does, but we want it to always be True now, whereas the internal method defaults to False

I was thinking more along the line of a one-line change here:

jdaviz/jdaviz/configs/default/plugins/subset_tools/subset_tools.py

Line 177 in e6548c4

'get_center', 'set_center',

oh that removes set_center entirely, we don't want to do that, we just don't want to expose the update keyword argument

did we decide that we didn't want it exposed at all anymore? Maybe I just misunderstood/misremembered

I thought we said it shouldn't be public API in the first place, right, @javerbukh ?

Yes it shouldn't be public since it doesn't really make sense as API. The Recenter button provides a new center (as an estimate) which is what the subset is then centered on. Using that method with API would require the user to come up with the center themselves and at that point they should just use the update subset public API (which does not exist yet).

ok, then I can revert that here and maybe we should wait until the alternative (upcoming) edit subset API is in place and then deprecate this method then?

This reverts commit 02b73db.

codecov · 2025-02-21T16:08:44Z

Codecov Report

Attention: Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 87.58%. Comparing base (dfe9a2f) to head (8f78d52).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
jdaviz/core/helpers.py	0.00%	2 Missing ⚠️

❌ Your patch status has failed because the patch coverage (60.00%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3453      +/-   ##
==========================================
- Coverage   87.59%   87.58%   -0.01%     
==========================================
  Files         128      128              
  Lines       20047    20042       -5     
==========================================
- Hits        17560    17554       -6     
- Misses       2487     2488       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pllim · 2025-02-21T19:22:07Z

jdaviz/configs/default/plugins/model_fitting/model_fitting.py

@@ -987,6 +987,35 @@ def get_model_parameters(self, models=None, model_label=None, x=None, y=None):

        return parameters_cube

+    def get_model_parameters(self, model_label=None, x=None, y=None):


Does this not need a change log?

I don't think so (see explanation in description), but can add it if you think there should

pllim

LGTM. Thanks!

javerbukh

Looks good, thanks!

kecnry added 3 commits February 19, 2025 14:06

explicitly mention region_format in import_region and remove **kwargs

bfee307

* if we have need for any kwargs passed through load_regions, we can explicitly include/document those as well

remove update=False kwarg from exposed set_center

02b73db

* user API is now hardcoded to update=True, which was not the previous default (BEHAVIOR CHANGE)

remove "models" from public API in model fitting

1ec16ab

* deprecation warning already exists for the old helper methods, and plugin APIs have not been released yet, so no need to add deprecations here

kecnry added this to the 4.2 milestone Feb 19, 2025

github-actions bot added testing imviz plugin Label for plugins common to multiple configurations labels Feb 19, 2025

kecnry added 3 commits February 19, 2025 14:21

codestyle

3bd5715

changelog entry

6ca0cd0

update test

d000bc3

kecnry marked this pull request as ready for review February 19, 2025 19:59

kecnry requested review from rosteen, javerbukh, pllim, bmorris3, haticekaratay, cshanahan1 and gibsongreen as code owners February 19, 2025 19:59

pllim reviewed Feb 19, 2025

View reviewed changes

Revert "remove update=False kwarg from exposed set_center"

8f78d52

This reverts commit 02b73db.

kecnry force-pushed the cleanup-plugin-apis branch from fb4285c to 8f78d52 Compare February 21, 2025 15:27

pllim reviewed Feb 21, 2025

View reviewed changes

pllim added the api API change label Feb 21, 2025

pllim approved these changes Feb 21, 2025

View reviewed changes

pllim added the Ready for final review label Feb 21, 2025

javerbukh approved these changes Feb 24, 2025

View reviewed changes

kecnry merged commit f759009 into spacetelescope:main Feb 24, 2025
31 of 33 checks passed

kecnry deleted the cleanup-plugin-apis branch February 24, 2025 15:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup plugin apis #3453

Cleanup plugin apis #3453

kecnry commented Feb 19, 2025 •

edited

Loading

pllim Feb 19, 2025

kecnry Feb 19, 2025

pllim Feb 19, 2025

kecnry Feb 19, 2025

kecnry Feb 19, 2025

pllim Feb 19, 2025

javerbukh Feb 21, 2025

kecnry Feb 21, 2025

codecov bot commented Feb 21, 2025 •

edited

Loading

pllim Feb 21, 2025

kecnry Feb 21, 2025

pllim left a comment

javerbukh left a comment

		@@ -987,6 +987,35 @@ def get_model_parameters(self, models=None, model_label=None, x=None, y=None):

		return parameters_cube

		def get_model_parameters(self, model_label=None, x=None, y=None):

Cleanup plugin apis #3453

Cleanup plugin apis #3453

Conversation

kecnry commented Feb 19, 2025 • edited Loading

Description

Change log entry

Checklist for package maintainer(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 21, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pllim left a comment

Choose a reason for hiding this comment

javerbukh left a comment

Choose a reason for hiding this comment

kecnry commented Feb 19, 2025 •

edited

Loading

codecov bot commented Feb 21, 2025 •

edited

Loading