Skip to content

collections.Collections offset improvements #20698

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

Closed
fourpoints opened this issue Jul 20, 2021 · 1 comment
Closed

collections.Collections offset improvements #20698

fourpoints opened this issue Jul 20, 2021 · 1 comment

Comments

@fourpoints
Copy link
Contributor

fourpoints commented Jul 20, 2021

Describe the issue

Summary

Make the behaviour for setting offsets in collections.Collection and subclasses more consistent and predictable.

Update 1: 2021-07-21 09:34: Added second fix proposal
Update 2: 2021-07-21 15:55: Added bug with lc.get_paths()


This is my second issue here, so if I'm proposing too much in a single issue, just close this and I'll make several smaller ones. If this looks okay, I'll make a PR with the proposed changes.

  • Setting offsets during initialization has a slightly different behavior from calling coll.set_offsets() after initialization.
    • If set during intialization, but without the transOffset parameter, then coll._uniform_offsets are set, otherwise this is None. Calling coll.set_offsets() when coll._uniform_offsets is None will update coll._offsets instead. This affects the results of coll.get_datalim().
    • coll._offsetsNone is set during initialization, but never updated if .set_offsets is used. (I could only find this used in coll.get_datalim(), but I couldn't produce any side effects.)
  • transOffset can only be set if offset is included during initialization, and is otherwise ignored without warning.
  • There is a coll.get_offset_transform(), but no coll.set_offset_transform().

Unrelated to collections:

  • One of the functions called in coll.get_datalim() has a typo:
    • _BlendedMixin.contains_branch_seperately and Transform.contains_branch_seperately are misspelled. (It should be separately.) (Separate issue?)

Different behaviour for setting offsets during and after initialization:

import matplotlib.patches as mpatches
import matplotlib.collections as mcollections
import matplotlib.transforms as mtransforms

patch = mpatches.Circle((0, 0), radius=5)

good = mcollections.PatchCollection([patch], offsets=(10, 10))

bad = mcollections.PatchCollection([patch])
bad.set_offsets((10, 10))

ugly = mcollections.PatchCollection([patch])
ugly._uniform_offsets = (10, 10)

identity = mtransforms.IdentityTransform()

print("good", good.get_datalim(identity))
print("bad", bad.get_datalim(identity))
print("ugly", ugly.get_datalim(identity))

output:

>>> good: Bbox(x0=-5.0, y0=-5.0, x1=5.0, y1=5.0)
>>> bad:  Bbox(x0=5.0, y0=5.0, x1=15.0, y1=15.0)
>>> ugly: Bbox(x0=-5.0, y0=-5.0, x1=5.0, y1=5.0)

Proposed fix

  1. Update the behavior of coll.set_offsets():
    • set coll._offsetsNone to False when called.
    • set coll._uniform_offsets instead of coll._offsets if coll._transOffset is None.
  2. Either add a warning, or set transOffset during initialization if passed without offsets.
  3. Add coll.set_offset_transform():
    • If coll._uniform_offsets is not None, then coll._offsets must be set to this value, and coll._uniform_offsets must be set to None.

I don't know what to do with the misspelled function names. Keep them, but make an alias?


Update 1: After a bit of digging, it appears that self._uniform_offsets are a remnant of LineCollection that was left behind after merging with Collection. (It first appeared here). So a second proposal would be to merge coll._offsets and coll._uniform_offsets. This would simplify coll.set_offsets and coll.get_offsets. The only other usage of coll._uniform_offsets in collections is in LineCollection._add_offsets.


I admit that I'm not certain what the use case for this is, but here is a possible issue with _uniform_offsets. lc.get_paths() (and lc.get_segments()). It will show different output depending on whether offsets are set during initialization or after. I'm not sure what the intended behaviour is, because lc._add_offsets is unclear to me.

import matplotlib.collections as mcollections

coll1 = mcollections.LineCollection([[[0, 0], [1, 1], [2, 2]], [[0, 0], [1, 2]]])
coll1.set_offsets([(1, 1)])

coll2 = mcollections.LineCollection([[[0, 0], [1, 1], [2, 2]], [[0, 0], [1, 2]]], offsets=[(1, 1)])

print(coll1.get_paths()[1])
print(coll2.get_paths()[1])

output

Path(array([[0., 0.],
       [1., 2.]]), None)
Path(array([[1., 1.],
       [2., 3.]]), None)
@QuLogic
Copy link
Member

QuLogic commented Aug 6, 2021

I think this is fixed by #20717?

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

No branches or pull requests

3 participants