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

Avoid double messaging on subset creation #2515

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

astrofrog
Copy link
Member

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

… followed by an update message, instead just broadcast a create subset message with the correct subset state
@astrofrog
Copy link
Member Author

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

@rosteen
Copy link
Contributor

rosteen commented Oct 1, 2024

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

dhomeier commented Oct 1, 2024

But nothing needed on this side, is that right?

@rosteen
Copy link
Contributor

rosteen commented Oct 1, 2024

But nothing needed on this side, is that right?

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

@rosteen
Copy link
Contributor

rosteen commented Oct 1, 2024

@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
Copy link
Collaborator

dhomeier commented Oct 2, 2024

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

assert isinstance(message.subset.subset_state, InequalitySubsetState)
assert message.subset.subset_state.left is data.id['x']
assert message.subset.subset_state.operator is operator.ge
assert message.subset.subset_state.right == 3
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
assert message.subset.subset_state.right == 3
assert message.subset.subset_state.right == 3
def test_broadcast_to_collection():
"""A data collection input works on each data component"""
handler = MagicMock()
app = Application()
class Listener(HubListener):
pass
listener = Listener()
app.session.hub.subscribe(listener, SubsetCreateMessage, handler=handler)
app.session.hub.subscribe(listener, SubsetUpdateMessage, handler=handler)
data = Data(x=[1, 2, 3, 4, 5])
data2 = Data(x=[2, 3, 4, 5, 6])
app.data_collection.append(data)
app.data_collection.append(data2)
cmd = ApplySubsetState(data_collection=app.data_collection,
subset_state=data.id['x'] >= 3,
override_mode=ReplaceMode)
app.session.command_stack.do(cmd)
assert len(data2.subsets) == 1
assert data2.subsets[0].label == 'Subset 1'
# fails with `IncompatibleAttribute` exception
# assert data2.get_subset_object(subset_id='Subset 1', cls=NDDataArray)
assert handler.call_count == 2
assert data2.subsets[0].subset_state.left is data.id['x']
assert data2.subsets[0].subset_state.operator is operator.ge
assert data2.subsets[0].subset_state.right == 3

basic test to check that subset is broadcast to a second component in the dataset.

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

Successfully merging this pull request may close these issues.

3 participants