| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
There was a problem hiding this comment.
While I've not tested this i have some reservations concerning performance. format_coord is called on mouse move and should be fast. Likely a worst-case scenario are 10x10 Axes without twinning, but using a Cursor so that delays become more apparent.
we should at least have an early return if there are no twins.
Sorry, something went wrong.
|
Sure, I added an early return for the twinless case. Edit: done as a separate commit: I switched Grouper so that it handles the ordering. |
Sorry, something went wrong.
There was a problem hiding this comment.
Just one typo.
This seems like a reasonable thing to do. I haven't tried to check the performance. Given that for a stream of mouse move events the twin lists and the transforms aren't going to change, there is considerable unnecessary work going on; but reducing it might be more trouble than it is worth.
Possible bike-shedding: change the joins from " / " to ", "? I think it would be more readable.
Sorry, something went wrong.
|
Did you try playing with it (e.g. plot(); twinx() and move the mouse around) with a ", " separator? I think " / " is more readable, but certainly that's subjective and I can go with whatever separator has the approval of most. Reducing the "duplicate" work seems not so easy without introducing some layers of caching and cache invalidation, which I'm not particularly keen to do. |
Sorry, something went wrong.
|
I just tested it out and it seemed pretty good performance wise to me. I would vote for pipe "|" instead of the slant for the separator. |
Sorry, something went wrong.
|
Sure, works for me. |
Sorry, something went wrong.
| # For each item, we store (order_in_which_item_was_seen, group_of_item), which | ||
| # lets __iter__ and get_siblings return items in the order in which they have | ||
| # been seen. |
There was a problem hiding this comment.
Is this necessary? If I understand this correctly, you just encode the insertion order into the WeakKeyDictionary. Can't we rely on order preservation there? I've not tested this thoroughly but the internal data format WeakKeyDictionary.data is a plain dict, which preserves order. So it should work as is. While WeakKeyDictionary has no official documentation whehter it preserves order or not, I assume CPython would not drop that de-facto feature without deprecation.
Sorry, something went wrong.
There was a problem hiding this comment.
Order preservation isn't really sufficient.
The mapping is storing {ax: [sibling0, sibling1, ...], ...} and what matters is that the list of siblings is sorted by the order in which they themselves occur in the mapping. So we could not explicitly store the index and instead sort the siblings by key=list(self._mapping).index (taking advantage of order preservation) before returning them but that's less efficient.
Sorry, something went wrong.
| " | ".join( | ||
| ax.format_xdata(ax.transData.inverted().transform(screen_xy)[0]) | ||
| for ax in sorted(y_twins, key=attrgetter("zorder"))), | ||
| " | ".join( | ||
| ax.format_xdata(ax.transData.inverted().transform(screen_xy)[1]) |
There was a problem hiding this comment.
Just realizing that this yield x=1 | 2 y=5, which is not very readable. In particular | creates a stronger visual separation than the space before y.
I suggest to regroup into x, y pairs (even if that means
I suggest to either: x=1 ; 2 | y=5.
Or regrouping into x, y pairs per Axes: x, y = (1, 5) | (2, 5)
Sorry, something went wrong.
There was a problem hiding this comment.
on the call we like x, y = (1, 5) | (4, 5) best and lean to changing the non-twinned version to be x, y = (1, 3)
Sorry, something went wrong.
|
This was discussed during the call today. Looks like the preferred option is Tim's last one ((x, y) = (x1, y1) | (x2, y2)), also changing for the non-twinned case and paying attention to make that also work for the 3d case. |
Sorry, something went wrong.
|
Meeting consensus: switch everything (regular and twinned) to @timhoffm's last suggestion above. |
Sorry, something went wrong.
|
🤣 Three of us posting notes about the meeting consensus. |
Sorry, something went wrong.
|
I implemented the (x, y) = (..., ...) | (..., ...) output format in the last commit, but it turns out that's quite wide and tends to overflow the toolbar (depending on the exact axes limits); perhaps the reviewers can give it a try. |
Sorry, something went wrong.
Sorry, something went wrong.
|
Did a rebase, partially to rerun tests since logs were removed. Test failures are real, and related to the change at hand. |
Sorry, something went wrong.
|
Fixed. |
Sorry, something went wrong.
|
Python 3.12 error seems related. |
Sorry, something went wrong.
|
Fixed for real this time? Sorry for the back and forth. |
Sorry, something went wrong.
|
I also figured out that it would be much cleaner for Grouper to keep track of the ordering info in a separate dict rather than sticking everything in _mapping; moved that to a new _ordering dict instead. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
PR Summary
Closes #4284, closes #5506 (also: #2986 (comment)).
PR Checklist
Documentation and Tests
Release Notes