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

Link mosviz data as a batch after they are loaded #762

Merged
merged 24 commits into from
Sep 7, 2021
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
cc13d6b
Add mosviz example notebook with updated nirspec data
javerbukh Jul 12, 2021
1f7604a
Link data as one batch rather than at data load
javerbukh Aug 5, 2021
93cd249
Remove unused file
javerbukh Aug 5, 2021
60ca838
Update notebooks with auto_link option
javerbukh Aug 5, 2021
aab771a
Move auto_link parameter to kwargs
javerbukh Aug 5, 2021
61932e9
Update jdaviz/app.py
javerbukh Aug 5, 2021
51352a7
Use link manager delay when linking Mosviz tables
ojustino Aug 18, 2021
7f63f27
Don't convert subsets to user-facing objects if they are incompatible
astrofrog Aug 24, 2021
c62fbdb
Fix codestyle checks
javerbukh Aug 26, 2021
4504a24
Accepted suggestions from code review
ojustino Aug 26, 2021
2572568
Implemented suggestions from @pllim
ojustino Aug 26, 2021
19ac2ad
Standardized Mosviz spelling in example notebooks
ojustino Aug 27, 2021
d5a6ba7
Move auto_link from app call to inside load_data method
javerbukh Aug 27, 2021
7939d2c
Clean up notebooks
javerbukh Aug 27, 2021
60ce746
Add comments
javerbukh Aug 30, 2021
5aa6080
Merge branch 'main' into better_linking
javerbukh Aug 31, 2021
dbafddf
Fix codestyle checks
javerbukh Aug 31, 2021
8486e82
Remove reference to auto_link from app calls
javerbukh Aug 31, 2021
7e2369e
Removed auto_link reference from notebook
ojustino Aug 31, 2021
37c9d34
Merge branch 'main' into better_linking
javerbukh Aug 31, 2021
8ef5878
Merge branch 'main' into better_linking
javerbukh Sep 1, 2021
8084945
Fix PEP 8
pllim Sep 2, 2021
6d25c70
Update jdaviz/configs/mosviz/plugins/parsers.py
rosteen Sep 7, 2021
592b3f6
Update jdaviz/configs/mosviz/plugins/parsers.py
rosteen Sep 7, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions jdaviz/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
from regions import RectanglePixelRegion, PixCoord
from specutils import Spectrum1D

from glue.core.exceptions import IncompatibleAttribute
from glue.config import data_translator
from glue.config import settings as glue_settings
from glue.core import BaseData, HubListener, Data, DataCollection
Expand Down Expand Up @@ -148,6 +149,10 @@ def __init__(self, configuration=None, *args, **kwargs):
# Parse the yaml configuration file used to compose the front-end UI
self.load_configuration(configuration)

# If true, link data on load. If false, do not link data to speed up
# data loading
self.auto_link = kwargs.pop('auto_link', True)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't actually work properly is auto_link is passed to the initializer as the auto_link option is first passed to the parent class in the super().__init__ call and is then not recognized. For this to work it has to come before the super() call I think.


# Subscribe to messages indicating that a new viewer needs to be
# created. When received, information is passed to the application
# handler to generate the appropriate viewer instance.
Expand Down Expand Up @@ -244,6 +249,11 @@ def _link_new_data(self):
any components are compatible with already loaded data. If so, link
them so that they can be displayed on the same profile1D plot.
"""
# Allow for batch linking of data in the parser rather than on
# data load
if not self.auto_link:
return

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
Expand Down Expand Up @@ -442,8 +452,11 @@ def get_data_from_viewer(self, viewer_reference, data_label=None,

if cls is not None:
handler, _ = data_translator.get_handler_for(cls)
layer_data = handler.to_object(layer_data,
statistic=statistic)
try:
layer_data = handler.to_object(layer_data,
statistic=statistic)
except IncompatibleAttribute:
continue

data[label] = layer_data

Expand Down
25 changes: 23 additions & 2 deletions jdaviz/configs/mosviz/helper.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ class MosViz(ConfigHelper):

_default_configuration = "mosviz"

def __init__(self):
super().__init__()
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)

spec1d = self.app.get_viewer("spectrum-viewer")
spec1d.scales['x'].observe(self._update_spec2d_x_axis)
Expand Down Expand Up @@ -131,6 +131,7 @@ def load_data(self, spectra_1d, spectra_2d, images=None, spectra_1d_label=None,
``images``. Can be a list of strings representing data labels
for each item in ``data_obj`` if ``data_obj`` is a list.
"""
self.app.auto_link = False

# If we have a single image for multiple spectra, tell the table viewer
if not isinstance(images, (list, tuple)) and isinstance(spectra_1d, (list, tuple)):
Expand All @@ -145,6 +146,21 @@ def load_data(self, spectra_1d, spectra_2d, images=None, spectra_1d_label=None,

self.load_2d_spectra(spectra_2d, spectra_2d_label)
self.load_1d_spectra(spectra_1d, spectra_1d_label)
self.link_table_data(None)

javerbukh marked this conversation as resolved.
Show resolved Hide resolved
self.app.auto_link = True

def link_table_data(self, data_obj):
"""
Batch link data in the Mosviz table rather than doing it on
data load.

Parameters
----------
data_obj : obj
Input for Mosviz data parsers.
"""
super().load_data(data_obj, parser_reference="mosviz-link-data")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just out of interest, what is the motivation for doing the linking as a parser as opposed to just a method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it was to keep all related code in one file and have the linking method be callable from the helper file, which (if I remember correctly) can only be done using parser_reference.


def load_metadata(self, data_obj):
"""
Expand Down Expand Up @@ -195,7 +211,12 @@ def load_2d_spectra(self, data_obj, data_labels=None):
data_labels=data_labels)

def load_niriss_data(self, data_obj, data_labels=None):
self.app.auto_link = False

super().load_data(data_obj, parser_reference="mosviz-niriss-parser")
self.link_table_data(data_obj)

self.app.auto_link = True

def load_images(self, data_obj, data_labels=None, share_image=0):
"""
Expand Down
37 changes: 37 additions & 0 deletions jdaviz/configs/mosviz/plugins/parsers.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
from astropy.wcs import WCS
from asdf.fits_embed import AsdfInFits
from pathlib import Path
from glue.core.link_helpers import LinkSame


__all__ = ['mos_spec1d_parser', 'mos_spec2d_parser', 'mos_image_parser']

Expand Down Expand Up @@ -87,6 +89,41 @@ def _fields_from_ecsv(fname, fields, delimiter=","):
return parsed_fields


@data_parser_registry("mosviz-link-data")
def link_data_in_table(app, data_obj=None):
"""
Batch links data in the mosviz table viewer.

Parameters
----------
app : `~jdaviz.app.Application`
The application-level object used to reference the viewers.
data_obj : None
Passed in in order to use the data_parser_registry, otherwise
not used.
"""
mos_data = app.session.data_collection['MOS Table']
wc_spec_ids = []

# Optimize linking speed through a) delaying link manager updates with a
# context manager, b) handling intra-row linkage of 1D and 2D spectra in a
# loop, and c) handling inter-row linkage after that in one fell swoop.
with app.data_collection.delay_link_manager_update():
for index in range(len(mos_data.get_component('1D Spectra').data)):
spec_1d = mos_data.get_component('1D Spectra').data[index]
spec_2d = mos_data.get_component('2D Spectra').data[index]
# image = mos_data.get_component('Images').data[index] # include?
rosteen marked this conversation as resolved.
Show resolved Hide resolved

wc_spec_1d = app.session.data_collection[spec_1d].world_component_ids
wc_spec_2d = app.session.data_collection[spec_2d].world_component_ids
# wc_image = app.session.data_collection[image].world_component_ids
# [RA, Dec] instead of [World]
rosteen marked this conversation as resolved.
Show resolved Hide resolved

wc_spec_ids.append(LinkSame(wc_spec_1d[0], wc_spec_2d[0]))

app.session.data_collection.add_link(wc_spec_ids)


@data_parser_registry("mosviz-spec1d-parser")
def mos_spec1d_parser(app, data_obj, data_labels=None):
"""
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,8 @@ def _on_viewer_data_changed(self, msg=None):
if msg is not None and msg.viewer_id != self._viewer_id:
return

self._viewer_data = self.app.get_data_from_viewer('spectrum-viewer')
self._viewer_data = self.app.get_data_from_viewer('spectrum-viewer',
include_subsets=False)

self.dc_items = [data.label
for data in self.app.data_collection
Expand Down
8 changes: 6 additions & 2 deletions jdaviz/configs/specviz/plugins/viewers.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from glue.core.subset import Subset
from glue.config import data_translator
from glue_jupyter.bqplot.profile import BqplotProfileView
from glue.core.exceptions import IncompatibleAttribute

import astropy
from astropy.utils.introspection import minversion
Expand Down Expand Up @@ -66,8 +67,11 @@ def data(self, cls=None):

if _class is not None:
handler, _ = data_translator.get_handler_for(_class)
layer_data = handler.to_object(layer_data,
statistic=statistic)
try:
layer_data = handler.to_object(layer_data,
statistic=statistic)
except IncompatibleAttribute:
continue
data.append(layer_data)

return data
Expand Down
4 changes: 2 additions & 2 deletions jdaviz/core/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,9 @@ class ConfigHelper(HubListener):
"""
_default_configuration = 'default'

def __init__(self, app=None):
def __init__(self, app=None, auto_link=True):
ojustino marked this conversation as resolved.
Show resolved Hide resolved
if app is None:
self.app = Application(configuration=self._default_configuration)
self.app = Application(configuration=self._default_configuration, auto_link=auto_link)
else:
self.app = app

Expand Down
14 changes: 7 additions & 7 deletions notebooks/MosvizExample.ipynb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
"cell_type": "markdown",
"metadata": {},
"source": [
"# MOSViz example notebook"
"# Mosviz example notebook"
]
},
{
Expand All @@ -28,7 +28,7 @@
"cell_type": "markdown",
"metadata": {},
"source": [
"This starts MOSViz."
"Next, start Mosviz. Set the `auto_link` argument to `False` to speed up its data loading process. (We'll encounter that later.)"
]
},
{
Expand All @@ -51,7 +51,7 @@
"source": [
"But before we can use it, we need some data.\n",
"\n",
"The MOSViz parsers accept lists of `Spectrum1D`, `SpectralCube`, and `CCDData` for 1D, 2D, and image data, respectively. Alternatively, users can also provide lists of file paths and MOSViz will internally attempt to parse them as their respective data types."
"The Mosviz parsers accept lists of `Spectrum1D`, `SpectralCube`, and `CCDData` for 1D, 2D, and image data, respectively. Alternatively, users can also provide lists of file paths and Mosviz will internally attempt to parse them as their respective data types."
]
},
{
Expand Down Expand Up @@ -108,7 +108,7 @@
"cell_type": "markdown",
"metadata": {},
"source": [
"Now that we have the data files extracted into `data_dir`, we can generate lists for MOSViz."
"Now that we have the data files extracted into `data_dir`, we can generate lists for Mosviz."
]
},
{
Expand Down Expand Up @@ -176,7 +176,7 @@
"cell_type": "markdown",
"metadata": {},
"source": [
"If no images are provided, MOSViz can still display the spectra."
"If no images are provided, Mosviz can still display the spectra."
]
},
{
Expand Down Expand Up @@ -236,7 +236,7 @@
],
"metadata": {
"kernelspec": {
"display_name": "Python 3",
"display_name": "Python 3 (ipykernel)",
"language": "python",
"name": "python3"
},
Expand All @@ -250,7 +250,7 @@
"name": "python",
"nbconvert_exporter": "python",
"pygments_lexer": "ipython3",
"version": "3.9.5"
"version": "3.8.11"
}
},
"nbformat": 4,
Expand Down
18 changes: 13 additions & 5 deletions notebooks/MosvizNIRISSExample.ipynb
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
"id": "2b2fcea7",
"metadata": {},
"source": [
"# MOSViz NIRISS example notebook"
"# Mosviz NIRISS example notebook"
]
},
{
Expand All @@ -32,7 +32,7 @@
"id": "e3d6f12f",
"metadata": {},
"source": [
"This starts MOSViz."
"Next, start Mosviz."
]
},
{
Expand All @@ -44,9 +44,9 @@
},
"outputs": [],
"source": [
"from jdaviz.configs.mosviz.helper import MosViz\n",
"from jdaviz.configs.mosviz.helper import MosViz as Mosviz\n",
"\n",
"mosviz = MosViz()\n",
"mosviz = Mosviz()\n",
"mosviz.app"
]
},
Expand Down Expand Up @@ -116,6 +116,14 @@
"source": [
"mosviz.load_niriss_data(data_dir)"
]
},
{
"cell_type": "code",
"execution_count": null,
"id": "03884229",
"metadata": {},
"outputs": [],
"source": []
}
],
"metadata": {
Expand All @@ -134,7 +142,7 @@
"name": "python",
"nbconvert_exporter": "python",
"pygments_lexer": "ipython3",
"version": "3.8.10"
"version": "3.8.11"
}
},
"nbformat": 4,
Expand Down