Skip to content

Conversation

@lkeegan
Copy link

@lkeegan lkeegan commented Jul 21, 2025

  • move these existing methods from LineGraphic to common base clase PositionsGraphic
  • regenerate docs

- move these existing methods from `LineGraphic` to common base clase `PositionsGraphic`
- regenerate docs
@kushalkolar
Copy link
Member

Hi thanks for your PR!

Supporting selections for scatters will also require implementing the methods to get data from the selections, for example with the rectangle selector:

I'm not sure what a linear or linear region selector means for a scatter, do you have a use case example? I can maybe see the usecase for exploring regression plots.

@lkeegan
Copy link
Author

lkeegan commented Jul 22, 2025

Hi thanks for your PR!

Supporting selections for scatters will also require implementing the methods to get data from the selections, for example with the rectangle selector:

Thanks for your feedback - and for this very nice library! I'll look into updating these methods.

I'm not sure what a linear or linear region selector means for a scatter, do you have a use case example? I can maybe see the usecase for exploring regression plots.

I guess if one axis in the scatter is time it could make sense to select a time range of data using the linear region selector.
My personal use case is very basic - I just want to be able to select a time on linked plots of time series data, e.g. a line plot of voltage traces and a scatter plot of features extracted from these traces:

plots.mp4

Copy link
Member

@clewis7 clewis7 left a comment

Choose a reason for hiding this comment

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

Hi @lkeegan

Thanks again for making a PR! I had a few small questions/tweaks. I'm sure @kushalkolar will also want to take a look. I checked out your examples, and they look great!


# min/max limits
limits = (x_axis_vals[0], x_axis_vals[-1], ymin * 1.5, ymax * 1.5)
limits = (xmin, xmax, ymin, ymax)
Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove the ymin * 1.5 and ymax * 1.5? I think we had that there to allow the selector to go a little bit above and below the graphic.

Copy link
Author

Choose a reason for hiding this comment

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

Ah sorry - I removed it because with positive ymin the limits didn't allow you to select all the data, restored in 75b7d73

Comment on lines 336 to 342
axis_vals_min = np.floor(axis_vals.min()).astype(int)
axis_vals_max = np.floor(axis_vals.max()).astype(int)

bounds_init = axis_vals_min, axis_vals_min + 0.25 * (
axis_vals_max - axis_vals_min
)
limits = axis_vals_min, axis_vals_max
Copy link
Member

Choose a reason for hiding this comment

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

Can you add code comments here?

@clewis7
Copy link
Member

clewis7 commented Jul 23, 2025

@kushalkolar LGTM as long as the examples pass

@kushalkolar
Copy link
Member

I'm going to wait until #837 is merged first since that changes some things with selectors.

@lkeegan
Copy link
Author

lkeegan commented Sep 30, 2025

@kushalkolar I've updated this PR to include the polygon selector from #837

@kushalkolar
Copy link
Member

Thanks for the update! I'll try this out and merge after #904 so that I don't have to replace the ground truth screenshots multiple times, it will be merged soon! 😄

self._resizable = bool(resizable)

BaseSelector.__init__(self, name=name, parent=parent)
self._move_info = MoveInfo("none", -1, -1, None, None)
Copy link
Member

Choose a reason for hiding this comment

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

@almarklein I'm guessing this was a typo? 😆

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think so, the type of MoveInfo.mode was str. Though in _end_move_mode I did set it to None, so I was not consistent 😆.

So this change looks fine to me, especially with the type now also being mode: str | None.

xmin = np.floor(x_axis_vals.min()).astype(int)
xmax = np.ceil(x_axis_vals.max()).astype(int)

# min/max limits include the data + 25% padding in the y-direction
Copy link
Member

Choose a reason for hiding this comment

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

might want to pad in both x and y for a scatter?

LinearRegionSelector which selects along the x-axis and a vertical selector which moves along the y-axis.
"""

# test_example = false
Copy link
Member

Choose a reason for hiding this comment

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

make this true, we'd want to have a ground truth screenshot for this since it's a new feature.

Example showing how to use a `PolygonSelector` with a scatter plot.
"""

# test_example = false
Copy link
Member

Choose a reason for hiding this comment

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

make this true, we'd want to have a ground truth screenshot for this since it's a new feature.

Example showing how to use a `RectangleSelector` with a scatter plot.
"""

# test_example = false
Copy link
Member

Choose a reason for hiding this comment

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

make this true, we'd want to have a ground truth screenshot for this since it's a new feature.

@kushalkolar kushalkolar mentioned this pull request Oct 1, 2025
14 tasks
@lkeegan
Copy link
Author

lkeegan commented Nov 10, 2025

@kushalkolar thanks for reviewing, I've made the suggested changes and merged the latest changes from the main branch

@kushalkolar
Copy link
Member

kushalkolar commented Nov 10, 2025 via email

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants