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

INFO: Subset API audit 2/4 (get, export, write) #3414

Open
pllim opened this issue Jan 30, 2025 · 1 comment
Open

INFO: Subset API audit 2/4 (get, export, write) #3414

pllim opened this issue Jan 30, 2025 · 1 comment

Comments

@pllim
Copy link
Contributor

pllim commented Jan 30, 2025

subset_tools.get_regions(): We want this to be the canonical API.

def get_regions(self, region_type=None, list_of_subset_labels=None,
use_display_units=False):

Returns
-------
regions : dict
A dictionary mapping subset labels to their respective ``regions``
objects (for spatial regions) or ``SpectralRegions`` objects
(for spectral regions).

jdaviz.app.get_subsets(): We want to be able to replace this with subset_tools.get_regions() or have subset_tools.get_regions() call this instead, but somehow it was not possible, why? Note that this method is way more complicated than the one in subset_tools and calls another recursive method and also is more powerful. Need to think how to consolidate the two.

jdaviz/jdaviz/app.py

Lines 921 to 924 in 4ffc3a0

def get_subsets(self, subset_name=None, spectral_only=False,
spatial_only=False, object_only=False,
simplify_spectral=True, use_display_units=False,
include_sky_region=False):

jdaviz/jdaviz/app.py

Lines 952 to 957 in 4ffc3a0

Returns
-------
data : dict
Returns a nested dictionary with, for each subset, 'name', 'glue_state',
'region', 'sky_region' (set to None if not applicable), and 'subset_state'.
If subset is composite, each constituant subset will be included individually.

jdaviz.app.get_sub_regions(): Only used internally in jdaviz.app.get_subsets(). Keep an eye out in case it is no longer necessary if we remove jdaviz.app.get_subsets().

jdaviz/jdaviz/app.py

Lines 1138 to 1139 in 4ffc3a0

def get_sub_regions(self, subset_state, simplify_spectral=True,
use_display_units=False, get_sky_regions=False):

export.save_subset_as_region() and export.save_subset_as_table(): Relies on jdaviz.app.get_subsets(). Should this use subset_tools API instead?

def save_subset_as_region(self, selected_subset_label, filename):

def save_subset_as_table(self, filename):

subset_tools._unpack_get_subsets_for_ui(): Relies on jdaviz.app.get_subsets(). Should this use subset_tools API instead?

def _unpack_get_subsets_for_ui(self):

jdaviz.core.region_translators._get_region_from_spatial_subset(): It is used internally in jdaviz.core.template_mixin.SubsetSelect._get_spatial_region(), aper_phot._background_selected_changed(), and subset_tools.recenter(). Should this function be retired? Can it be replaced by subset_tools API?

def _get_region_from_spatial_subset(plugin_obj, subset_state):

Already deprecated

@deprecated(since="4.1", alternative="subset_tools.get_regions")
def get_interactive_regions(self):

@deprecated(since="4.1", alternative="subset_tools.get_regions")
def get_spectral_regions(self, use_display_units=False):

🐱

@pllim pllim changed the title INFO: Subset API audit 2/4 (export, write) INFO: Subset API audit 2/4 (get, export, write) Jan 30, 2025
@pllim
Copy link
Contributor Author

pllim commented Jan 31, 2025

Done. Open to comments.

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

No branches or pull requests

1 participant