spacetelescope / jdaviz

JWST astronomical data analysis tools in the Jupyter platform
https://jdaviz.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
134 stars 71 forks source link

Have spectral extraction api be more "findable" and intuitive method names #1600

Open eteq opened 2 years ago

eteq commented 2 years ago

This is a follow-on from #1554, which I should have put as a comment there but I missed the boat and didn't get in a review before it was merged.

The core issue is twofold:

  1. I think viz.app.get_tray_item_from_name('spectral-extraction') is a very hard to find way of accessing this functionality. It's more internally consistent, but it makes the "helper" not very helpful.
  2. I think the names import_* and export_*, while descriptive, aren't necessarily intuitive in how a user would actually use it.

So my proposal for the fix would be twofold:

  1. Have some sort of syntactic sugar from the specviz2d helper that would let you do viz.extraction - probably that could just be a simple:
    @property
    def extraction(self):
    return self.app.get_tray_item_from_name('spectral-extraction')
  2. Rename the import/export methods to something that are more like action verbs for what the user is doing. I.e., instead of import_trace it could be load_trace_into_viewer, and instead of export_extract it could be get_extraction_object.

It might even make sense for this to be a whole layer of syntactic sugar - i.e. viz.extraction.load_trace_into_viewer could be a thin wrapper calling viz.app.get_tray_item_from_name('spectral-extraction').import_trace, although that feels a bit like overkill to me (i.e., better to just decide which name of the methods is best and switch to that)

🐱

kecnry commented 2 years ago

1639 would address the first of the two points, by exposing viz.plugins['Spectral Extraction']. Since spectral extraction is a core-functionality to specviz2d (unlike other plugins in which the app should be able to function if they are removed), we could still consider a helper-level property as a shortcut to this.