-
Notifications
You must be signed in to change notification settings - Fork 59
Allow use of add_*_selector methods in ScatterGraphic
#883
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
base: main
Are you sure you want to change the base?
Allow use of add_*_selector methods in ScatterGraphic
#883
Conversation
- move these existing methods from `LineGraphic` to common base clase `PositionsGraphic` - regenerate docs
|
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. |
Thanks for your feedback - and for this very nice library! I'll look into updating these methods.
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. plots.mp4 |
clewis7
left a comment
There was a problem hiding this 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| 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 |
There was a problem hiding this comment.
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?
…e unused variables
…rk when ymin is positive
|
@kushalkolar LGTM as long as the examples pass |
|
I'm going to wait until #837 is merged first since that changes some things with selectors. |
|
@kushalkolar I've updated this PR to include the polygon selector from #837 |
|
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) |
There was a problem hiding this comment.
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? 😆
There was a problem hiding this comment.
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.
fastplotlib/graphics/scatter.py
Outdated
| 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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
…etting selection to complete selection
|
@kushalkolar thanks for reviewing, I've made the suggested changes and merged the latest changes from the main branch |
|
Thanks I haven't forgotten this! We're just very busy with other parts of
the library at the moment.
…On Mon, Nov 10, 2025, 09:33 Liam Keegan ***@***.***> wrote:
*lkeegan* left a comment (fastplotlib/fastplotlib#883)
<#883 (comment)>
@kushalkolar <https://github.com/kushalkolar> thanks for reviewing, I've
made the suggested changes and merged the latest changes from the main
branch
—
Reply to this email directly, view it on GitHub
<#883 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACHXXRCGFODJ34AGDBLM2YL34CO5DAVCNFSM6AAAAACB76T6VWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTKMJSGA2DOMRRGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
LineGraphicto common base clasePositionsGraphic