Link mosviz data as a batch after they are loaded #762
Conversation
|
A couple errors that show up with this implementation: Long traceback after a subset is added in the specviz viewer and another row is selected
Subset highlight on the spectrum also does not appear. |
| for index in range(0, 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] | ||
| app.session.data_collection.add_link(LinkSame(spec_1d, spec_2d)) |
There was a problem hiding this comment.
Is there a need to check if that link already exists before actually adding it?
There was a problem hiding this comment.
I'm not sure, I can look into how to do that though. I think we can assume that someone using Mosviz will have auto_link set to False since it speeds up data loading considerably, but you know what they say about assumptions...
Codecov Report
@@ Coverage Diff @@
## main #762 +/- ##
==========================================
+ Coverage 67.20% 67.26% +0.06%
==========================================
Files 65 65
Lines 4619 4650 +31
==========================================
+ Hits 3104 3128 +24
- Misses 1515 1522 +7
Continue to review full report at Codecov.
|
|
@javerbukh said @ojustino is taking over this one and will push a fix for the subset bug. |
|
As a progress report, interestingly enough, I'll continue testing in the meantime. |
|
I had assumed that the previous commits had disabled auto-linking in Mosviz by default. I can confirm that setting I can load 20 spectra with cutouts in the time it takes to load 15 with I tested a possible solution to the subset selection error in Unfortunately, it still leads to similar results as before -- no highlighted region and the following (different) traceback:
Also, these extra links noticeably slow |
|
|
||
| wc_spec_1d = app.session.data_collection[spec_1d].world_component_ids | ||
| wc_spec_2d = app.session.data_collection[spec_2d].world_component_ids | ||
| app.session.data_collection.add_link(LinkSame(wc_spec_1d[0], wc_spec_2d[0])) |
There was a problem hiding this comment.
This is a better linking approach than what is in main at the moment, but you can then get significantly better performance for this step if you also use the context manager I used in #782 - could you test this out?
There was a problem hiding this comment.
Now that #782 is merged you can encapsulate the whole for loop in the delay context manager
There was a problem hiding this comment.
OK, I see it now. The gain in speed for using the delay context manager here (close to an order of magnitude for link_table_data() on its own with 20 spectra + images) is similar to what we found with a different approach of appending the LinkSame object from each iteration of this loop to a list and waiting until after the loop to call add_link() on that list.
Combining these approaches is even faster. Are any of these strategies better or worse from a glue perspective?
There was a problem hiding this comment.
Combining the two approaches makes sense from a glue perspective!
ojustino
force-pushed
the
better_linking
branch
from
db262b7 to
abe02b5
Compare
|
The latest commit uses the delay link manager and other adjustments to make further speed enhancements to It works for me, but a similar traceback to the one I included in my Aug 9 comment still persists on the creation of a subset. Whatever is happening also now prevents the image and spectra from changing even after I click a new table row. |
|
Investigating! |
|
I found an issue which I've fixed in javerbukh#8 - let me know if there are still issues after this |
|
Pulling javerbukh#8 resolves the issue for me in However, the pull request does not fix the error I reported earlier when I use a notebook from Patrick that loads newer simulated data for NIRSpec. If it's a problem with the data, I'm not sure why that would affect @javerbukh I'd be interested to see how using this pull request affects your experience with the |
javerbukh
requested review from
duytnguyendtn,
ibusko and
rosteen
as code owners
There was a problem hiding this comment.
Works wonderfully! Anecdotally, both our NIRISS and sample datasets load SO much faster. Great work to all involved!
There is one small code comment I'd like to get in here that I feel particularly strong about, but I'm happy to approve if you all wouldn't mind my pedantic nature!
|
@ojustino I noticed that when loading NIRISS data, the image viewer is a black screen. Do you see this same interaction? If so, could it be because we are not linking the image data? |
|
@javerbukh Did you check your layer scaling? The NIRISS data was almost black upon initial load for me other than a couple stars, had to rescale to see much. |
|
I won't give a formal review since this is partially my ticket, but I'll add the following:
@javerbukh I do see the "Image canucs F150W" layer in the image viewer. The color is stretched such that the background is black instead of gray, but I can pick out a few of the brighter sources. |
There was a problem hiding this comment.
After catching up on the conversation for this one, I can agree with @ojustino that selecting subsets in the spectral viewers is still broken. While I am nervous about this, crunch time is starting to settle in, and I can be convinced of filing a bug ticket and addressing this afterwards. I'm also debating whether to hold this off because this one is an improvement over an improvement and we could do without it, but I suspect if we don't address this now, this can get quickly forgotten. I'm going to approve this for now, should another dev agree to move forward with the bug ticket (particularly since we have #733 in progress as well)
EDIT: I should also add, I tested Cubeviz, Imviz, and Specviz, all of which seem to be unaffected
EDIT2: Clarifying point, I'm still seeing this subset issue with the NIRISS dataset
|
So, I guess there are still some unresolved issues w.r.t. this PR, as I can see in #762 (comment) . We should not merge until we have a clear path forward with this (either fix it here or defer as future work). Also my comment about the commented code still stands. |
|
I am now investigating issues related to the subset selection. |
There was a problem hiding this comment.
A couple of comments below - the main one being that MosViz(auto_link=False) doesn't work.
@ojustino - I looked into the notebook you sent and it looks like the subset issue is related to the fact that the 2D spectrum WCS is incorrect:
Number of WCS axes: 3
CTYPE : 'RA---TAN' 'DEC--TAN' 'WAVE'
CRVAL : 215.0466793383765 52.90137035377627 2.4999999999999998e-06
CRPIX : -0.00020768428328117 -7.3732734554624e-07 1024.0
PC1_1 PC1_2 PC1_3 : -1.0 0.0 0.0
PC2_1 PC2_2 PC2_3 : 0.0 1.0 0.0
PC3_1 PC3_2 PC3_3 : 1.0 0.0 0.0
CDELT : 2.96691831370797e-05 1.05359563567715e-07 6.72e-12
NAXIS : 436 25 1The PC matrix is not correct as the PC?_3 elements are all zero which means that the WAVE axis isn't correlated connected with the first pixel axis of the data array. This in turn causes all kinds of problems in the linking. I think this is a byproduct of setting up the fake 3D WCS in the parsers, which would be solved by moving away from SpectralCube as done in #733 so I think the best approach might be to go ahead and merge this and I can then rebase #733 and make sure it works with the two standard notebooks and the one you sent, for the subsets.
I'm happy to approve this once the minor issue of the initializer option is missing.
| @@ -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) | |||
There was a problem hiding this comment.
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.
| data_obj : obj | ||
| Input for Mosviz data parsers. | ||
| """ | ||
| super().load_data(data_obj, parser_reference="mosviz-link-data") |
There was a problem hiding this comment.
Just out of interest, what is the motivation for doing the linking as a parser as opposed to just a method?
There was a problem hiding this comment.
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.
|
This has two approvals, but before I hit the merge button I want to make sure I'm clear about the status of the subset bug(s?) mentioned. I see a bug using this PR that I don't see on main: when using the x-range tool in the 1D viewer, if I then drag the selected region around to move the gray region selection box, it creates a bunch of new subsets instead of applying the moved selection to the same subset. Is that what you think will be fixed by #733 @astrofrog ? Or was there another subset bug being discussed separate from the one I described? |
|
I think any subset issue here can be ignored in favour of dealing with it in 733 as they are likely all link related. |
Co-authored-by: P. L. Lim <[email protected]>
Co-authored-by: P. L. Lim <[email protected]>
|
I committed @pllim 's code suggestions to remove commented-out image linking code, since it seems the consensus is to not link the images here. I'll merge this once the CI passes again...this just became relevant for something else I'm working on so I want to get it in. |
Description
Creates an option in the application initialization to turn off auto linking of data when it is loaded into the application. Mosviz utilizes this in this PR by linking data as a batch after it is loaded into the table. Only the data in a row is linked, although this does lead to problems with subsets. However, data loading is significantly improved and no longer has an exponential increase in load time depending on the number of data loaded.
Depends on:
Follow-up after merge:
Fixes #430
Checklist for package maintainer(s)
This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.
triviallabel.CHANGES.rst?