spacetelescope / jdaviz

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

Data linking in ApplicationState._link_new_data is too specific to live in Application #801

Open astrofrog opened 3 years ago

astrofrog commented 3 years ago

The following code in Application:

https://github.com/spacetelescope/jdaviz/blob/d296c6312b020897034e9dd1fc58c84a2559efa5/jdaviz/app.py#L241-L260

is actually wrong for some applications such as MOSViz. It will link the first world coordinates of all datasets, regardless of whether the data are images or spectra and so on. This means that some links will end up being Declination and Spectral axis and so on.

I think this code made sense for e.g. specviz and probably cubeviz but it is too specific to live in the main Application (and is incorrect for some applications)

pllim commented 3 years ago

Since we actually store the config, probably not hard to only run this if certain viz is loaded:

https://github.com/spacetelescope/jdaviz/blob/d296c6312b020897034e9dd1fc58c84a2559efa5/jdaviz/app.py#L1205-L1206

astrofrog-conda-forge commented 3 years ago

I don't think we want to have it live on the main class and optionally select it based on the configuration - after all that is what the subclasses are for - so it could live on the relevant applications. I don't think the default application should do any auto linking.

pllim commented 3 years ago

I see people calling Application(configuration) in their notebooks. I don't think subclass will work.

astrofrog commented 3 years ago

Ah I see - then yes maybe there should be an option for this, but in any case the way the function is written is prone to breakage so needs to be improved to more explicitly link spectral coordinates. I am writing up an issue about improving linking across the board, I will touch on that function in there.

pllim commented 3 years ago

xref #804