Avoid double messaging on subset creation

[astrofrog]

Avoid broadcasting a create subset message with an empty subset state followed by an update message, instead just broadcast a create subset message with the correct subset state

[astrofrog]

@dhomeier - would you like to review this? I've had confirmation from the jdaviz side that it works for them.

[rosteen]

Actually, maybe hold off on merging - I think we might need a small PR in Jdaviz, I'm seeing spectral extraction fail to extract when a spatial subset is created. Hopefully a quick fix on our end.

[dhomeier]

But nothing needed on this side, is that right?

[rosteen]

But nothing needed on this side, is that right?

I'm pretty sure that's correct, still investigating the fix on our end.

[rosteen]

@astrofrog It seems like at the point we get the SubsetCreate message, the subset hasn't finished propagating/being applied to all of the data - when I switched to subscribing to SubsetCreate for spectral extraction line 441 in spectral_extraction.py is causing our automatic extraction upon making a subset to fail:

uncertainties = uncert_cube.get_subset_object(
                        subset_id=aperture.selected, cls=StdDevUncertainty
                    )

with the error

File [~/projects/glue/glue/core/data.py:330](http://localhost:8888/lab/tree/~/projects/glue/glue/core/data.py#line=329), in BaseData.get_subset_object(self, subset_id, cls, **kwargs)
    326         raise ValueError('Specify the object class to use with cls= - supported '
    327                          'classes are:' + format_choices(data_translator.supported_classes))
    329 if len(self.subsets) == 0:
--> 330     raise ValueError("Dataset does not contain any subsets")
    331 elif subset_id is None:
    332     if len(self.subsets) == 1:

ValueError: Dataset does not contain any subsets

It seems like this is a timing conflict, would it be possible to send the SubsetCreate message after the subset has propagated to all of the relevant data? Or do you have a workaround you could recommend on our side?

[dhomeier]

The setter is already calling self._broadcast() afaics; any ideas if adding something in BaseData.add_subset could help?

[astrofrog]

@rosteen - I have now added some changes to make sure that we delay any subset creation or deletion messages until the subset has been defined or removed from all datasets. Would you be able to take this for a spin?

@dhomeier - do you think we still need the extra test you suggested above?

[rosteen]

@rosteen - I have now added some changes to make sure that we delay any subset creation or deletion messages until the subset has been defined or removed from all datasets. Would you be able to take this for a spin?

Great, I'll try to test it today before I'm off for vacation.

[dhomeier]

I've tested this with the Cubeviz example, which seems to work – uncertainty subsets are updated on main; automated spectral extraction still needs spacetelescope/jdaviz#3238 (subscribe(SubsetCreateMessage), I assume) to work.

@dhomeier - do you think we still need the extra test you suggested above?

Not sure; functionality within datasets seemed a bit undertested in general, but since your test caught the actual issue, that's obviously the more important one.
Looking back at the Slack thread, I was originally trying to test the extraction using get_subset_object the way it is done in Jdaviz' spectral_extraction, but could not find a way to get that to work within the test setup. @kecnry wanted to make sure the subsets are all "fully populated" once broadcast, so still seems reasonable to test that all properties match for the different datasets?

[rosteen]

@rosteen - I have now added some changes to make sure that we delay any subset creation or deletion messages until the subset has been defined or removed from all datasets. Would you be able to take this for a spin?

Great, I'll try to test it today before I'm off for vacation.

It looks like this works now, extracted spectra are getting created automatically as expected with spacetelescope/jdaviz#3238.

[astrofrog]

@dhomeier sounds good, could you push your test as a commit to this PR?

[dhomeier]

Ah, in the commit view it looked like test_broadcast_to_collection was already included in 36b74e2 – can simply commit from the suggestion.

[dhomeier]

Hmm

        assert len(data1.subsets) == len(data2.subsets)
>       assert data2.subsets[0].subset_state is data1.subsets[0].subset_state
E       IndexError: tuple index out of range

any idea why this is passing in a local run, but failing in all CI envs??

[astrofrog]

@dhomeier - I think that error makes sense once the subset is removed as there should no longer be any subsets. Maybe you should have an if len(data1.subsets) > 0 before that assert?

[dhomeier]

Obviously; that's what you get from not properly syncing your local branch ;-)
I think they are still planning to make some tests in spacetelescope/jdaviz#3238, but this looks good to go now.