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

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

Open
astrofrog opened this issue Aug 24, 2021 · 5 comments
Labels

Comments

@astrofrog
Copy link
Collaborator

The following code in Application:

jdaviz/jdaviz/app.py

Lines 241 to 260 in d296c63

def _link_new_data(self):
"""
When additional data is loaded, check to see if the spectral axis of
any components are compatible with already loaded data. If so, link
them so that they can be displayed on the same profile1D plot.
"""
new_len = len(self.data_collection)
# Can't link if there's no world_component_ids
wc_new = self.data_collection[new_len-1].world_component_ids
if wc_new == []:
return
# Link to the first dataset with compatible coordinates
for i in range(0, new_len-1):
wc_old = self.data_collection[i].world_component_ids
if wc_old == []:
continue
else:
self.data_collection.add_link(LinkSame(wc_old[0], wc_new[0]))
break

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
Copy link
Contributor

pllim commented Aug 24, 2021

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

jdaviz/jdaviz/app.py

Lines 1205 to 1206 in d296c63

# store the loaded config object
self._loaded_configuration = config

@astrofrog-conda-forge
Copy link

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
Copy link
Contributor

pllim commented Aug 25, 2021

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

@astrofrog
Copy link
Collaborator Author

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
Copy link
Contributor

pllim commented Aug 25, 2021

xref #804

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants