ENH: Implemented y-axis-based labeling#136

Open

pevoz23 wants to merge 6 commits intocphyc:main from

pevoz23:yaxis

Open

ENH: Implemented y-axis-based labeling #136
pevoz23 wants to merge 6 commits intocphyc:main from
pevoz23:yaxis

Conversation

@pevoz23

Copy link

@pevoz23 pevoz23 commented May 8, 2023

Should close #121

dperozzi and others added 6 commits

May 8, 2023 17:40

@dperozzi


 ENH: Implemented y-axis-based labeling

e853d34

Should close cphyc#121

@pre-commit-ci


 [pre-commit.ci] auto fixes from pre-commit.com hooks

b5467e6

for more information, see https://pre-commit.ci

@dperozzi


 Fixing pre-commit-ci errors

f70d2e9

Adjusted docstring to desired maximum line length

@pre-commit-ci


 [pre-commit.ci] auto fixes from pre-commit.com hooks

ef52007

for more information, see https://pre-commit.ci

@dperozzi


 Update Readme.md to consider new argument names

fda7f37

Changed the `xvals` argument to `vals` to reflect the changes made in commit e853d34

@dperozzi


 Completely removed the variable name xval in favor of val

e2bc243

cphyc

cphyc requested changes

May 9, 2023

View reviewed changes

Copy link

Owner

@cphyc cphyc left a comment •

edited

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

labellines/core.py

calls to e.g. legend do not use it anymore.

yoffset : double, optional

offset : double, optional

Space to add to label's y position

Copy link

Owner

@cphyc cphyc May 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

Space to add to label's y position

Space to add to label's x/y position

labellines/core.py

Comment on lines +36 to +37

axis : "x" | "y"

Reference axis for `val`.

Copy link

Owner

@cphyc cphyc May 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

labellines/core.py

drop_label=False,

shrink_factor=0.05,

yoffsets=0,

offsets=0,

Copy link

Owner

@cphyc cphyc May 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above about maintaining backward compatibility.

labellines/core.py

align=True,

xvals=None,

vals=None,

axis=None,

Copy link

Owner

@cphyc cphyc May 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should have the same signature as above (i.e. axis defaults to x).

labellines/core.py

Comment on lines +114 to +115

axis : None | "x" | "y", optional

Reference axis for the `vals`.

Copy link

Owner

@cphyc cphyc May 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once you update the signature, don't forget to update the docstring as well ;)

labellines/core.py

Comment on lines +139 to +143

if axis == "y":

yaxis = True

else:

axis = "x"

yaxis = False

Copy link

Owner

@cphyc cphyc May 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change

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'")

labellines/core.py

Comment on lines 185 to +189

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]

Copy link

Owner

@cphyc cphyc May 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't implement the logic for the y axis.

@pevoz23

Copy link

Author

pevoz23 commented May 12, 2023

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.

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).

@kavanase

Copy link

kavanase commented Aug 17, 2024

Just to bump, this would be a very useful feature if it could be included!
Our use case is for labelling the chemical formulae of materials in phase diagrams (used in computational materials science research), and we have some smart algorithms for picking the label positions, but for vertical lines we can't set y so it always places the label at the y-midpoint which isn't always desirable (e.g. "P" here):
image

image

@cphyc

Copy link

Owner

cphyc commented Aug 19, 2024

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.

@kavanase

Copy link

kavanase commented Jun 21, 2025

@pevoz23 do you have the time to implement the review comments on this?

Labels

None yet

4 participants

@pevoz23 @kavanase @cphyc @dperozzi

Conversation

@pevoz23 pevoz23 commented May 8, 2023

Uh oh!

@cphyc cphyc left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@cphyc cphyc May 9, 2023

Choose a reason for hiding this comment

Uh oh!

@cphyc cphyc May 9, 2023

Choose a reason for hiding this comment

Uh oh!

@cphyc cphyc May 9, 2023

Choose a reason for hiding this comment

Uh oh!

@cphyc cphyc May 9, 2023

Choose a reason for hiding this comment

Uh oh!

@cphyc cphyc May 9, 2023

Choose a reason for hiding this comment

Uh oh!

@cphyc cphyc May 9, 2023

Choose a reason for hiding this comment

Uh oh!

@cphyc cphyc May 9, 2023

Choose a reason for hiding this comment

Uh oh!

pevoz23 commented May 12, 2023

Uh oh!

kavanase commented Aug 17, 2024

Uh oh!

cphyc commented Aug 19, 2024

Uh oh!

kavanase commented Jun 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

@cphyc cphyc left a comment •

edited

Loading