Format ticks for non-SI-unit axis in a more readable way #1243
Conversation
There was a problem hiding this comment.
Just to clarify, for unit-less data, do users also want to get kilo/micro/femto stuff in the labels? If so, then this commit is not enough. If no, then I'm confused about what is going on.
In any case, don't forget to:
- update the tests (see travis)
- update docstrings in plotting.py file to reflect that something else happens in case of unit-less data
- perhaps, update the example notebook (most probably "offline plotting tutorial")
|
I am also confused 😃 |
|
Sorry I overlooked that you @astafan8 were so thorough with your tests ;-). But you are right maybe this should live somewhere else, so that we get nice axis exponentials. Let me play around a little and see where to put it. (And it does not need to be a manual formater necessarily).
|
Dominik-Vogel
changed the title
|
I added standard formatting now where the plots get generated. |
|
@Dominik-Vogel looks great! Concerning the placement of 1e-5 I think it would make sense to place inside the unit brackets as it really is part of the unit after rescaling: Flux (1e-5 e^2/hbar). |
|
@ThorvaldLarsen I totally agree. But my impresion is that we should prioritize it lower as the plotting front end will change soon and it requires some extra tweaking. If it is very important to you, then we can certainly change it. |
|
Not really important. Was only if it was a quick change. |
|
@Dominik-Vogel there is some problem in CI when the notebook gets executed for docs. |
|
@astafan8 yes I noticed... It is quite cumbersome.... it comes from an example where two graphs are plotted into the same axes.... Maybe it is after all easier to do it as Thorvald suggested.... |
|
Hopefully the final version... I now use the same logic as is used for the SI units but instead of prepending the unit character (k, M, T...) I prepend the exponent (always going to the closest multiple of 3):
(Yes If this works I will update the docstring, just to say before you do it @astafan8 ;-)) |
|
please add tests if you have time ;) |
qcodes/dataset/plotting.py
Outdated
| `_UNITS_FOR_RESCALING`. | ||
| The units for which unit prefixes ared added can be found in | ||
| `_UNITS_FOR_RESCALING`. For all other units an exponential sacling factor | ||
| is added to the label i.e. `(10^3 x e^2/hbar)`. |
qcodes/dataset/plotting.py
Outdated
| return f'{label} ({unit})' | ||
| label = f'{label}' | ||
| if unit != '' and unit is not None: | ||
| label += f' ({unit})' |
| if postfix != '': | ||
| assert f"{base_label} ({postfix})" == label | ||
| else: | ||
| assert f"{base_label}" == label |
There was a problem hiding this comment.
the test method became too complex: it basically mirrors the logic of the function under test. Such a test becomes difficult to read and more difficult to modify. Let's be careful next time. As the initial author of the test, i should've also been more careful.
There was a problem hiding this comment.
Good point. Thanks for pointing that out, I got carried away ;-)
|
Hypothesis prints a warning that we probably need to fix |
|
@Dominik-Vogel Do you want to simplify the tests or should we just merge as is? |
Codecov Report
@@ Coverage Diff @@
## master #1243 +/- ##
==========================================
+ Coverage 70.64% 70.67% +0.03%
==========================================
Files 74 74
Lines 8160 8165 +5
==========================================
+ Hits 5765 5771 +6
+ Misses 2395 2394 -1 |
|
@jenshnielsen I would love to merge it already.... I think the tests are not great but ok... |
With @astafan8 fantastic PR #1239 the axis in
plot_by_idare scaled just like it was the case for the old dataset.This PR fixes the case of unit less axis, that has not been touched in the named PR. With this PR these get scaled and displayed in the same way as all others.
@nataliejpg