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

Fix incompatible attribute #8

Merged

Conversation

astrofrog
Copy link

Subsets that can't be computed for a given dataset shouldn't be converted to e.g. Spectrum1D and so on.

@astrofrog
Copy link
Author

Sorry rebase issue, please hold...

@astrofrog
Copy link
Author

This is ready for review by the way (the rebase has been fixed)

try:
layer_data = handler.to_object(layer_data,
statistic=statistic)
except IncompatibleAttribute:
Copy link

Choose a reason for hiding this comment

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

Looks awfully similar to glue-viz/glue-astronomy#39 but I think we're patching different things?

Copy link
Author

Choose a reason for hiding this comment

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

Ah yes it does seem like the same thing. I think we do want to handle things better in glue-astronomy but I think we still want to raise an error there rather than silently fail, and we do want to catch those exceptions here.

Copy link
Author

Choose a reason for hiding this comment

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

So I would suggest we go ahead with this PR, modify the glue-astronomy one to raise a better exception, and then update the exception we catch in jdaviz once glue-astronomy is released.

Copy link
Owner

@javerbukh javerbukh left a comment

Choose a reason for hiding this comment

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

This works, thank you!

@javerbukh javerbukh merged commit 9caa0c6 into javerbukh:better_linking Aug 26, 2021
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