You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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:
set coll._uniform_offsets instead of coll._offsets if coll._transOffset is None.
Either add a warning, or set transOffset during initialization if passed without offsets.
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.
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.
offsets
during initialization has a slightly different behavior from callingcoll.set_offsets()
after initialization.transOffset
parameter, thencoll._uniform_offsets
are set, otherwise this isNone
. Callingcoll.set_offsets()
whencoll._uniform_offsets
isNone
will updatecoll._offsets
instead. This affects the results ofcoll.get_datalim()
.coll._offsetsNone
is set during initialization, but never updated if.set_offsets
is used. (I could only find this used incoll.get_datalim()
, but I couldn't produce any side effects.)transOffset
can only be set ifoffset
is included during initialization, and is otherwise ignored without warning.coll.get_offset_transform()
, but nocoll.set_offset_transform()
.Unrelated to collections:
coll.get_datalim()
has a typo:_BlendedMixin.contains_branch_seperately
andTransform.contains_branch_seperately
are misspelled. (It should be separately.) (Separate issue?)Different behaviour for setting offsets during and after initialization:
output:
Proposed fix
coll.set_offsets()
:coll._offsetsNone
toFalse
when called.coll._uniform_offsets
instead ofcoll._offsets
ifcoll._transOffset
isNone
.transOffset
during initialization if passed withoutoffsets
.coll.set_offset_transform()
:coll._uniform_offsets
is notNone
, thencoll._offsets
must be set to this value, andcoll._uniform_offsets
must be set toNone
.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 ofLineCollection
that was left behind after merging withCollection
. (It first appeared here). So a second proposal would be to mergecoll._offsets
andcoll._uniform_offsets
. This would simplifycoll.set_offsets
andcoll.get_offsets
. The only other usage ofcoll._uniform_offsets
incollections
is inLineCollection._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()
(andlc.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, becauselc._add_offsets
is unclear to me.output
The text was updated successfully, but these errors were encountered: