Conversation
Should close cphyc#121
for more information, see https://pre-commit.ci
Adjusted docstring to desired maximum line length
for more information, see https://pre-commit.ci
Changed the `xvals` argument to `vals` to reflect the changes made in commit e853d34
There was a problem hiding this comment.
Thanks for the implementation! This looks good overall, but I have slight concerns about backward compatibility. Also, would it be possible to add test(s) for this? To do so, please read pytest-mpl doc and add the reference image in labellines/baseline.
Note about tests: there is some tiny variation from one run to another on Github Action, leading to some tests failing. I haven't been able to troubleshoot it, but you can keep an eye on the tests results found in the artifacts of the runs.
| calls to e.g. legend do not use it anymore. | ||
| yoffset : double, optional | ||
| offset : double, optional | ||
| Space to add to label's y position |
There was a problem hiding this comment.
| Space to add to label's y position | |
| Space to add to label's x/y position |
| axis : "x" | "y" | ||
| Reference axis for `val`. |
There was a problem hiding this comment.
I am overall happy with the val/axis combination. Two notes though
- we need to keep the old (
yoffset,yoffset_logspace) for backward compatibility - add a note in the docstring below to explain how to use the axis kwa?
| drop_label=False, | ||
| shrink_factor=0.05, | ||
| yoffsets=0, | ||
| offsets=0, |
There was a problem hiding this comment.
Same comment as above about maintaining backward compatibility.
| align=True, | ||
| xvals=None, | ||
| vals=None, | ||
| axis=None, |
There was a problem hiding this comment.
This should have the same signature as above (i.e. axis defaults to x).
| axis : None | "x" | "y", optional | ||
| Reference axis for the `vals`. |
There was a problem hiding this comment.
Once you update the signature, don't forget to update the docstring as well ;)
| if axis == "y": | ||
| yaxis = True | ||
| else: | ||
| axis = "x" | ||
| yaxis = False |
There was a problem hiding this comment.
Since the check below will be performed at different locations (here and at line 174), I'd suggest factorising it somehow.
I would also recommend explicitly testing the axis to be either x, y or otherwise fail with an error.
| if axis == "y": | |
| yaxis = True | |
| else: | |
| axis = "x" | |
| yaxis = False | |
| if axis == "y": | |
| yaxis = True | |
| elif axis == "x": | |
| yaxis = False | |
| else: | |
| raise ValueError(r"Got an invalid axis {axis}, expected 'x' or 'y'") |
| xscale = ax.get_xscale() | ||
| if xscale == "log": | ||
| xvals = np.logspace(np.log10(xmin), np.log10(xmax), len(all_lines) + 2)[ | ||
| 1:-1 | ||
| ] | ||
| vals = np.logspace(np.log10(vmin), np.log10(vmax), len(all_lines) + 2)[1:-1] | ||
| else: | ||
| xvals = np.linspace(xmin, xmax, len(all_lines) + 2)[1:-1] | ||
| vals = np.linspace(vmin, vmax, len(all_lines) + 2)[1:-1] |
There was a problem hiding this comment.
This doesn't implement the logic for the y axis.
Hey! Thanks for your review! I agree with all your comments and will work on them as soon as possible (likely in the next few days). |
|
Just to bump, this would be a very useful feature if it could be included! 
|
|
I'd be happy for someone to review the PR once the comments have been fixed (this one, or an offspring of it retaining authorship to @pevoz23) but I unfortunately do not have the time to do this myself. |
|
@pevoz23 do you have the time to implement the review comments on this? |
4 participants
Should close #121