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

Set better starting xlim and ylim for custom viewers #2422

Merged
merged 1 commit into from
Aug 10, 2023

Conversation

jfoster17
Copy link
Member

@jfoster17 jfoster17 commented Jun 27, 2023

Pull Request Template

Description

As pointed out in #2420 the documentation example for a Custom Viewer (the Shot Chart) does not work well with current versions of glue because the Shot Chart x and y limits are set to 0 - 1, which contains no data. For new users this looks like it's just not working. This fix does not require any changes to the example scripts; we add a custom add_data function that uses the Matplotlib bounding box to set some sensible initial limits if we haven't already set limits (which the example later does in a setup function).

Note that we can't use axes.relim() for this because hexbin returns a Collection which is not supported.

Closes #2420

@jfoster17 jfoster17 marked this pull request as draft August 2, 2023 18:15
@jfoster17 jfoster17 closed this Aug 2, 2023
@jfoster17 jfoster17 reopened this Aug 2, 2023
@jfoster17 jfoster17 marked this pull request as ready for review August 2, 2023 18:16
Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

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

Looks good to me! Just a quick question below.

self.state.x_max = self.axes.dataLim.xmax
self.state.y_min = self.axes.dataLim.ymin
self.state.y_max = self.axes.dataLim.ymax
self.limits_to_mpl()
Copy link
Member

Choose a reason for hiding this comment

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

Just curious, why is this needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, which line(s) are you asking about? The general idea is that without something like this the limits in the state object stay at the default values of (0,1) for custom viewers rather than getting updated when data (which is normally handled by limit helpers).

Copy link
Member

Choose a reason for hiding this comment

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

I meant the last line - since we are updating the state from the Matplotlib viewer, why do we then need to convert the state limits back to Matplotlib? Is it in case we make any changes in the state because of aspect ratio/etc?

Copy link
Member

Choose a reason for hiding this comment

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

In any case I'll go ahead and merge, we can always tweak later if you think this isn't needed but I don't think there's any harm in doing it.

@astrofrog astrofrog merged commit 620465f into glue-viz:main Aug 10, 2023
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.

Custom data viewer is not working
2 participants