GeezerVison

Does anybody have an idea what is causing the Latex errors (RuntimeError: LaTeX was not able to process the following string: '$$')?

I am not sure how this is related to the change I submitted (ticker.py is not in the stack trace). However, other PR's do not have this issue so it must be something in my code.

That is a random failure not related to anything in your PR, sorry about that. Did you intend to close the your pull request?

Sorry I was confused. We indeed have a number of random issues like that. But I don't think thats what happens here since it looks like it fails on all python versions

I closed the PR by accident. Nevertheless, I got the tests working locally, so I can fix some of the issues offline. When I have resolved the things I can fix myself, I will either make a new PR or ask for help. :)

No need to make a new PR; just push to the same branch,

I need some guidance: Due to a change in the code (for the better) some of the image tests/comparisons are failing. If an offset is used this is indicated at +1E10 and to make thing more symmetric scaling is now indicated with "·1E10" (note the cdot, previously none). This obviously causes the image tests to fail (see below).

Removing the cdot is an option, however, I think it really improves readability.

What is the next step? Should I change the test-images to include the cdot?

Completely off topic: what is a good toolset allowing one to switch between a devel version of matplotlib next to a normal version? I have been playing with virtualenv and will try conda next.

This is a case where replacing the test images makes sense.

I think a 'x' would make more sense than cdot. When I looked at the images before I read your text I thought the issue was that the minus sign was getting cut off!

I have used both venv and conda for flipping between mpl versions and either works. Conda in great on linux because there are binaries (when I was using venv, pip did not stash the bdist wheels it would build so it would have to reinstall from scratch. This was also way before I discovered ccache). conda also provides a version of qt/pyqt so you do not have to include system python packages, which is great if you want to flip numpy versions as well.

Thanks for the input! I changed the cdot to a cross, tacaswell's comment that the cdot could be mistaken for something else was something I did not consider.

I also changed the test images. To pass the test_axes.py test I re-implemented the option to have both an offset and a scaling factor. Why not support it, although it is not a normal use case (I hope, it hurts my brain).

Lets see what else I goofed up, my local test passed except for some unrelated errors with the svg and ps backend.

Sorry, I am having a lot of trouble following the changes here (hence my verbose questions).

I am not convinced that the logic of 'allowed to use an offset' and 'there is on offset worth using' can be merged into a single boolean because it should be responsive to scale changes. I am also not convinced that the handling of the rcparam is consistent with current behavior.

@tacaswell I gave your remark some thought. The logic existing before this fix was very opaque and scattered over multiple functions (good) in an illogical way (bad). For example, _set_offset should be named something along _calculateAutomaticOffset and should not contain logic at various positions to determine if that offset should be applied. That logic should be centralized somewhere else. Something similar holds for the very long _set_orderOfMagnitude(). So far I changed what was needed to get the desired functionality to work (see below). Maybe I should have started with a thorough cleanup.

I am willing to do the refactoring making the functions single purpose with the fewest possible side effects (as I have read up on the current code anyway), making the code in the ScalarFormatter more readable. On the other hand, this means that the change gets bigger and that will probably take more reviewing effort (although with better readable code). Before I start on this, I would like to know if there is support for such a step.

If we decide to refactor, you do not need to bother with the text from here on out.

Below I have previously written a clarification to the old/existing code and the new bits in reply to your post.

The code still uses the same/old functions for determining the scaling factor the offset implemented by [698] _set_orderOfMagnitude() and [667]_set_offset() respectively. Note that set_offset() is called at plot time and does not set the offset! I did not change any logic to determine the automatic behavior.

There are 3 booleans used to get the desired behavior (_usingScaling, _usingOffset, and _scientific) on top of the axes.formatter.useoffset and axes.formatter.limits. Moreover, orderOfMagnitude is used for the scaling value and offsetval is used for the offset value. As scientific notation is a specific case of a scaling factor, I used the majority of those functions. The only new parameter is _useScaling.

I have described the flow for several use cases to demonstrate that the rc-params are honored. Starting with the offset

1. No user input

• [655] set_locs() is called at plot time -> this calls [667] _set_offset()
• [667]_set_offset() evaluates the RC-param on [671].
• If axes.formatter.useoffset == False the offset is not set, method return. Therefore the _usingOffset remains false
• [606] get_offset() is called at plotting to add the label with the offset. This returns an empty string because _usingOffset is false.
• If axes.formatter.useoffset == True the old algorithm to determine the offset is used.
• [763] tick manipulation takes into account the offset

2. User input with set_useOffset(True) or set_useOffset(value)

• [655] set_locs() is called at plot time -> this class [667] _set_offset() which returns because of if statement [671]. User has set the values
• [606] get_offset() is called at plotting which returns the user set offset value
• [763] tick manipulation takes into account the offset

3. User input with set_useOffset(False)

• Identical to case 1), therefor honors axes.formatter.useoffset

For the scaling factor:

1. No user input

• [655] set_locs() is called at plot time -> this calls [698] _set_orderOfMagnitude()
• [698]_set_orderOfMagnitude() evaluates the RC-param self._powerlimits = rcParams['axes.formatter.limits']
  on [719] and [721] and does what it has always doen
• [606] get_offset() is called at plotting which returns a scaling factor determined with the already existing algorith. If the data is within axes.formatter.limits no scaling is applied.
• [763] tick manipulation takes into account the scaling factor

2. User input set_useScalingFactor(True) or set_useScalingFactor(value)

• [655] set_locs() is called at plot time -> this calls [698] _set_orderOfMagnitude()
• [698]_set_offset() returns because of user input on line [672]
• [606] get_offset() is called at plotting which returns the offset value set by the user
• [763] tick manipulation takes into account the scaling factor

3. User input set_useScalingFactor(True)

• Identical to case 1), therefor honors axes.formatter.limits

See above remark

@anntzer Is also working on a major re-write of some of these code paths (#5804 and
#5785)

I am tentatively in favor of a major refactor. Given the importance of this code to many users day-to-day use I am sure it got written very early and has grown very organically over time. Which is to say, that it is a part of the code base that could most benefit from an overhaul

That said, this is also critical code path for most users day-to-day plotting which means even small API breaks can cause major disruption. Which means it is part of code base I am least excited about overhauling.

I think #5785 (better choice of offset-text) is in pretty good shape and brings some helpful user-facing improvements; I'd like to see it (or some variant) merged. #5804 (complete rewrite of the formatter API) is also fine, but it's basically going to be very hard to get a decent rewrite of the mess that the formatter API is without breaking at least some obscure back-compatibilities, so I more or less gave up on it for now.

Closing as this has now been over a year without an update.

@VincentVandalon Thank you for you work and sorry this got stalled in review. If you are still interested in working on this please comment!