API Proposal: Importing/loading spatial and spectral regions

[pllim]

Scope: Propose API to import/load spatial and spectral regions.

Out of scope:

• Importing/loading composite/masked subsets/regions.
• Exporting any region.

🐱

Accepted plan

See #2871 (comment)

Spatial Regions

Existing API

These work in Cubeviz and Imviz.

viz_helper.load_regions_from_file()

jdaviz/jdaviz/core/helpers.py

Lines 643 to 644 in 069834a

	def load_regions_from_file(self, region_file, region_format='ds9', max_num_regions=20,
	**kwargs):

viz_helper.load_regions()

jdaviz/jdaviz/core/helpers.py

Lines 675 to 676 in 069834a

	def load_regions(self, regions, max_num_regions=None, refdata_label=None,
	return_bad_regions=False, **kwargs):

To export

viz.app.get_subsets()

jdaviz/jdaviz/app.py

Lines 949 to 952 in 069834a

	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):

viz.get_interactive_regions()

jdaviz/jdaviz/core/helpers.py

Line 861 in f985a7f

def get_interactive_regions(self):

Proposed API

No change; same as Existing API. One could argue to absorb load_regions_from_file into load_regions but both have existed for a while now and no one complained about it, so no motivation for extra churn.

Spectral Regions

Existing API

This theoretically works on any viz with a spectrum viewer.

from glue.core.roi import XRangeROI
sv = specviz_helper.app.get_viewer('spectrum-viewer')
sv.apply_roi(XRangeROI(6500, 7400))
viz_helper.app.state.drawer = True

To export

viz.app.get_subsets()

jdaviz/jdaviz/app.py

Lines 949 to 952 in 069834a

	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):

Proposed API 1

Same as spatial regions:

• viz_helper.load_regions_from_file()
• viz_helper.load_regions()

Pros: Less API for user to remember.

Cons: Possibility of over-complicating the logic in those methods due to different usages and file formats between spatial and spectral regions. Might also have to rename get_* methods.

Proposed API 2

Similar to spatial regions but not identical:

• viz_helper.load_spectral_regions_from_file()
• viz_helper.load_spectral_regions()

Pros: Cleanly separating out logic for spatial and spectral. User's intention is clear.

Cons: Possibility of code duplication where logic is shared between spatial and spectral regions. User has to remember to use different APIs for spatial vs spectral. Might have to rename spatial region methods by adding spatial to them.

Proposed API 3

The concept was originally suggested by @kecnry: The idea is we use plugin API directly. Example usage would be something like this (such functionality currently does not exist):

plg = viz.plugins["Subset Tools"]

plg.dataset = "some_image[SCI,1]"  # Might need this for Imviz due to linking galore

plg.import_region("subset1a.reg")
plg.combine_mode = "or"
plg.import_region(subset_1b_region_object)

plg.subset = "Create New"
plg.import_region("subset2.reg")

# Presumably combine_mode would be ignored
# and each region becomes a new Subset?
plg.import_region("lots_of_regions.reg")

Pros: "import" might be okay instead of "load" for plugin method since we also have "export" plugin. The API is controlled by plugin, so no need to choose between attaching it to helper or viewer.

Not-a-con-but-also-not-a-pro: We have to implement the functionality in the plugin first to support the API and possible new GUI elements to go with it.

Cons: Unclear what the behavior should be when a region file has multiple regions to be loaded via this plugin API, since the regions package does not understand glue Subset operations. For Imviz, behavior can be confusing when user changes Orientation between importing regions via this API. We have to deprecate load_* and get_* methods in helpers.

[javerbukh]

Of all of these options, I prefer API 1. Although, I think with version 4.0 we have an opportunity to make setting and retrieving subsets more straightforward. As a fresh user, I would assume the API for creating and retrieving subsets would look like create_subset and get_subset. Instead, we have load_regions, load_regions_from_files, get_subsets, and get_interactive_regions.

I would propose moving that functionality into one method for creating subsets, one for creating them from a file, and one for retrieving them for version 4.0. It looks like get_subsets already has a large amount of the functionality for retrieving subset data covered, and we can use the return values from that method as a test case for loading subsets in a create_subsets method.

So the two different methods for creating subsets would look like:

def create_subsets(self, subset_type='Spectral', subset_name=None, bounding_box)

and

def create_subsets_from_file(region_file, region_format='ds9', max_num_regions=20, **kwargs)

I know I took Proposal 1 and turned it into 10 sentences instead of 1 but I think renaming the method is an important part of clarifying the API for a user. Happy to hear your thoughts on this!

[pllim]

@javerbukh , would an API called create_subsets being unable to generate complicated composite subsets programmatically be confusing?

[javerbukh]

I know we do not support that currently but I do think it is possible to do programmatically. Should that be a blocker for this effort?

[pllim]

Should that be a blocker for this effort?

🤷‍♀️

I am not sure if it is possible until we have a way to serialize the operators into something regions can understand? That part is currently not existing.

Maybe @camipacifici can chime in here.

Also, regular users not familiar with glue internals might find "regions" more intuitive than "subsets".

[camipacifici]

Thoughts on proposed API:

• I believe the spectral equivalent of get_interactive_regions is get_spectral_regions. I would prefer not to rely on the methods hidden under app.
• I like the idea of copying the spatial approach and use load relying on the formats from photutils and specutils. I am assuming the spectral equivalent would be a SpectralRegion.

I like the idea of a method called create_subsets (or create_regions) that is more similar to what the user sees in the UI, but then the spatial case would need to change as well. It is important to keep them consistent. Either load or create.
I agree with Pey Lian that regions might be more intuitive than subset, but please check with Jenn. She might have some insights after the usability testing.

It would be ideal to have the possibility of combining regions to create composite regions. It doesn't have to be MVP, but definitely something to plan. Maybe it could be a separate method. The user could load individual regions and then combine them according to the usual arithmetic we have in the UI. Just an idea.

[pllim]

Just to make sure I understand, is this what you all are counter-proposing?

• Just one method to rule load them all: viz.load_regions()
  • We deprecate viz.load_regions_from_file().
• This method can load from filename, one regions object, a list of regions objects, one SpectralRegion object, or a list of SpectralRegion objects.
  • Is mixing them up allow? That is, can someone give a mixed list of filename(s), regions, and SpectralRegion objects all in one list?
• This method should be able to support creating composite Subset from input(s) in the future.
• When API settles from the initial discussions, we should ask Jenn Kotler for UX feedback.
  • How should this be presented to Jenn? POC PR? More pseudocode?

[pllim]

And just to be clear, I am not touching any export functions/methods. So design of viz.get_* and viz.app.get_* is irrelevant here. (I just added the "export" stuff in original post for completeness.)

[javerbukh]

It makes sense to have a separate method for loading with a file since the code will pretty different. My main point is that the naming convention should be consistent for all things that touch subsets/regions. I think using regions instead of subsets in the method names is fine, as long as it is consistent.

[pllim]

In that case, are we converging on Proposed API 1 as-is, @javerbukh and @camipacifici ?

[camipacifici]

If the get_ remain untouched (i.e., two different calls for spatial and spectral), then probably we should have different methods for loading spatial and spectral region as well. Unless, the get_ calls will also change to a global get_regions in the future. This is a dev decision though. Maybe poll the others and let me know what you all think is best here.

In any case, I think we are settling on the load approach with the possibility of combining loaded regions as a different method. It doesn't need to be implemented right away, but it would be useful to start thinking about the API for this too.

[pllim]

The ticket only asked to ponder about loading, not getting. If you want to refactor get_* too then we have to expand the scope of the ticket or create a follow-up ticket. 🤷‍♀️

[camipacifici]

I didn't ask to ponder about how to refactor get_. I asked to poll the other devs to see if they are in favor or against refactoring get_ in the future. If they are in favor, load_ can be a single method. If they are against, load_ should follow get_. Or whatever else you suggest we should do as long as they are (or will be) consistent.

[pllim]

OK thanks for the clarification. Two 👍 so far from devs to refactor get_ in the future. But I will wait a bit more.

[pllim]

Re: #2871 (comment)

@camipacifici , still just the 2 👍 and no 👎 . So does this mean we go with Proposed API 1?

[pllim]

Update: I added Proposed API 3 based on discussions this morning.

[camipacifici]

Thank you for this other proposal. Will there be similar calls for export_region when the user creates them in the UI? Will those replace get_*?

The UI functionality of the subset plugin should be improved regardless, so I do not see this as a problem among your cons. For the other cons, I rely on you all.

Both API1 and API3 look ok to me from a user stand point. Please advise on what would be best technically.

[pllim]

Thank you for this other proposal. Will there be similar calls for export_region when the user creates them in the UI? Will those replace get_*?

Good question and probably @kecnry can chime in here. A little weird for plugin API to export to object too when it can already export to files (standalone app users get no benefit from this) but it does put all the related functionality in one place.

I don't see the get_* stuff going anywhere. They are not public anyway. I think we just won't rename them nor make them public.

[kecnry]

If we go with API3, yes, I think it would make sense to have an export_region method in the subset plugin API.

We could also consider accessing these through the export plugin so that you can access any object directly from the export API instead of or in addition to writing to the file... we already have the logic there to determine the format).

[pllim]

I was thinking export_region to be in Export plugin but now I am not sure. Should we group all the region stuff into Subset Tools plugin, or group them by import/export? 🤔

[pllim]

When it comes the time people can add/remove plugins at will, does that mean they can no longer do these things unless those plugins are enabled (as opposed to attaching the API to helper/viewer)?

[kecnry]

I was thinking export_region to be in Export plugin but now I am not sure. Should we group all the region stuff into Subset Tools plugin, or group them by import/export?

I would not put any regions/subset-specific API into the export plugin, but we can use the general export method to also expose the underlying objects instead of only supporting writing to files.

When it comes the time people can add/remove plugins at will, does that mean they can no longer do these things unless those plugins are enabled (as opposed to attaching the API to helper/viewer)?

Yes, but I don't see any issue with that myself 🤷‍♂️

[pllim]

expose the underlying objects instead of only supporting writing to files.

Sure, but as far as the user is concerned, then exporting region API would be in the Export plugin. Unless you are saying we do that in addition to also putting export regions API in Subset Tools plugin?

[kecnry]

I think both is probably fine (subset plugin for now for the basic functionality, and export plugin down the road for translating to different formats, bulk retrieval, etc).

[camipacifici]

Looks like API3 is good. I re-send my question about the get_* methods because they are public. I am thinking get_interactive_regions and get_spectral_regions. Will they stay or will they go?

[pllim]

get_interactive_regions

Since I was the one that wrote this one, I am okay to deprecate it in favor of the new API. "interactive" is kinda meaningless now that we don't advertise loading static masks.

get_spectral_regions

I cannot speak for this one. @kecnry last touched it in #2127 . I am thinking we can also deprecate it unless other people object.

[pllim]

But this also means with Option 3, we also have to deprecate viz.load_regions and viz.load_regions_from_file, is that correct?

[camipacifici]

Right, those too.

[pllim]

OK I updated pros and cons of Option 3 to include deprecation of all those APIs. Seems pretty disruptive but if people really like the plugin API idea, we have little choice.

[rosteen]

Seems pretty disruptive

This does worry me a bit. I still like option 1, but can see the arguments for 3 as well. So I guess don't count me as a strong vote either way 😬. (so helpful)

[pllim]

I think at tag-up today, we chose Proposed API 3, so here is the plan. We can choose to implement new things first and deprecated later for smaller pointed tickets. We can also choose to split import and export work into different tickets. Though doing them all at once might be cleaner. Proposed new API signatures are negotiable.

	New functionality	Deprecation	No change
Import	subset_plugin.import_region(region_file_or_regions) subset_plugin.combine_mode = "operation_name" subset_plugin.dataset = "data_label" # Imviz only	viz.load_regions(regions) viz.load_regions_from_file(region_file) imviz.load_data(region_file)	viewer.apply_roi()
Export	subset_plugin.export_region(region_type, list_of_subset_labels, region_file)	viz.get_interactive_regions() viz.get_spectral_region()	app.get_subsets() export_plugin.export() export_plugin._obj.save_subset_as_region() export_plugin._obj.save_subset_as_table()

The plan:

1. User API and possible refactor of subset plugin 🐱
2. Subset Plugin API: Import single region from file, combination mode 🐱
3. Subset Plugin API: Import multiple regions from a single file, deprecate others 🐱
4. Subset Plugin API: Retrieve Subsets as regions, deprecate others 🐱
5. Subset Plugin UI: Import region(s) from file 🐱
6. ???
7. Profit!!!!