Fix prop cycling not working #26
Conversation
|
I would propose to change the cycle function to a cycle class and handle the cycling through there. This monkey patch here relies too much on internal mpl functions/variables and could be changed in the future. |
|
Now the cycling is more robust, and passing the tests without checking with baseline compare. Will do it later. |
|
Pinging @beckermr @pratiman-91 |
cvanelteren
changed the title
|
Could we go ahead and merge this? @pratiman-91 @beckermr |
|
I am out on holiday until the first week of Feb. If someone else can review. |
beckermr
left a comment
beckermr
left a comment
There was a problem hiding this comment.
Verifying that the Cycler object is correctly instantiated by our sublcass is difficult. I'd be good to add specific unit tests of this so that we verify it is correct and we can catch regressions. Ideally, we'd write the tests against a working version of the factory function first and then move the change to a class to a new PR and ensure the tests are still passing.
Using #20, we can check for validity. That is, as soon as we have main producing a good baseline, any further tests can be used to check whether changes to the codebase will produce the same validity as we will expect (using pytest-mpl). Currently main does not pass these tests, but I am expecting that the joint of #26, #24 will. |
|
I do like the idea of just testing if cycling happens correctly. I will write a simple test to see if it does that is lower level than the high level tests proposed above. |
|
Should be good now. An important step is to work towards a baseline that we can stand by. Proplot did this implicitly with the doctests. We want to move towards pyest-mpl as it gives a pixel perfect match rather than judging it by eye. For now these tests will fail. As noted above I think the current open prs will place us in a good spot moving forward. |
Have fun😬 |
cvanelteren
left a comment
cvanelteren
left a comment
There was a problem hiding this comment.
Applied all fixed, added unittests, fixed up doc strings
|
|
cvanelteren
force-pushed
the
cycling-fix
branch
from
dad47f0 to
6a67e88
Compare
cvanelteren
force-pushed
the
cycling-fix
branch
from
ae23719 to
330b9df
Compare
|
Rebased to changes to clean up the mess -- should be working now. I cleaned up the logic inside I'll put it on the todo to check what is causing this. |
|
I am not happy with how I cycle the properties here: there are calls to private properties and I cycle the object manually. After digging a bit, I can see that there is currently no public api for accessing the cycling object (see matplotlib/matplotlib#26831 (comment), EDIT: there is also an open issue matplotlib/matplotlib#19479). This leads us at a crossroad
UltraPlot does override some public function that exist already (e.g. see I am also not sure if this goes beyond this PR -- intended as a quick fix -- rather than a larger api change. Open for suggestions. |
|
Also see matplotlib/matplotlib#29469 |
|
Then we could add a test this future PR with def test_manual_cycle():
cycle = uplt.Cycle(
["red", "green", "black"],
marker=["X", "o"],
sizes=[20, 100],
edgecolors=["r", "k"],
)
fig, ax = uplt.subplots()
ax.set_prop_cycle(cycle)
ax.scatter([1], [1])
ax = ax[0] # force to get single axis rather than GridSpec
parser = ax._get_lines
current_pos = parser._idx
cycled_pos = (current_pos + 1) % len(parser._cycler_items)
props = ax._active_cycle.get_next()
assert cycled_pos == parser._idx
assert props == parser._cycler_items[parser._idx]or something similar |
|
I belief the root cause of the baseline images not showing the proper formatting when using pytest-mpl and they look fine when manually plotting is that in pytest-mpl they remove the |
|
I am merging this with devel. After locally testing the open PRs it all looks good for now. |
|
is that ok @beckermr? |
beckermr
left a comment
beckermr
left a comment
There was a problem hiding this comment.
This PR needs changes. Please wait next time.
|
|
||
| if resolved_cycle and resolved_cycle != self._active_cycle: | ||
| self.set_prop_cycle(resolved_cycle) | ||
|
|
||
| # Apply manual cycle properties | ||
| if cycle_manually: | ||
| current_prop = self._get_lines._cycler_items[self._get_lines._idx] | ||
| self._get_lines._idx = (self._get_lines._idx + 1) % len(self._active_cycle) | ||
| for prop, key in cycle_manually.items(): | ||
| if kwargs.get(key) is None and prop in current_prop: | ||
| value = current_prop[prop] | ||
| kwargs[key] = pcolors.to_rgba(value) if key == "c" else value |
There was a problem hiding this comment.
With the new changes, this block of code is skipped if return_cycle is True. Why is that?
| maxlen = np.lcm.reduce(lengths) | ||
| props = {key: value * (maxlen // len(value)) for key, value in props.items()} | ||
| mcycler = cycler.cycler(**props) | ||
| super().__init__(mcycler) |
There was a problem hiding this comment.
Can you add a comment on what this call does exactly? Does it use matplotlib's init function to update the current cycle to the computed one?
There was a problem hiding this comment.
cycler.cycler is not a class but a function in mpl. The logic of the init from the Cycle class is not compatible with the function -- that is we need to pass the output of cycler directly to the parent of the class we introduced.
There was a problem hiding this comment.
Cool. Let's add a comment on this in the code.
|
Ah my bad! |
|
I will open another PR to fix the fixes. |
|
Thank you! Sorry for the delay in my code review. |
|
No worries ;-) |
3 participants
Addresses #25
Note this is currently a draft as we are directly interfacing with private functions and this is not preferred.But it does solve the issue. I will give it some thought on how this could be solved.