Skip to content

Conversation

@seisman
Copy link
Member

@seisman seisman commented Oct 8, 2024

Description of proposed changes

Currently, Figure.text allows passing a list of non-string values into the text parameter, e.g., text=[1, 2, 3.0, 4.123, 5.000]. Internally, these non-string values are converted to an array of strings:

text = np.atleast_1d(text).astype(str)

Instead of np.atleast_1d(text).astype(str), we can also use np.atleast_1d(np.asarray(text, dtype=str)). The former converts text into a numpy array (float64 in this case) first and then converts it to str dtype; the latter converts text into str dtype directly.

Below are their differences:

In [1]: import numpy as np

In [2]: x = [1, 2, 3.0, 4.05, 5.123, 6.0000]

In [3]: np.atleast_1d(x).astype(str)
Out[3]: array(['1.0', '2.0', '3.0', '4.05', '5.123', '6.0'], dtype='<U32')

In [4]: np.atleast_1d(np.asarray(x, dtype=str))
Out[4]: array(['1', '2', '3.0', '4.05', '5.123', '6.0'], dtype='<U5')

In [5]: %timeit np.atleast_1d(x).astype(str)
4.57 μs ± 227 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)

In [6]: %timeit np.atleast_1d(np.asarray(x, dtype=str))
3.8 μs ± 102 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)

In [7]: np.atleast_1d(x).astype(str).itemsize
Out[7]: 128

In [8]: np.atleast_1d(np.asarray(x, dtype=str)).itemsize
Out[8]: 20

The latter is faster (3.8 μs vs 4.57 μs) and the resulting array is smaller (20 bytes vs 128 bytes per element).

Also please pay attention to the different string representations. For an integer 1, the former will typeset 1.0 while the latter will typeset 1. I think we can declare it as a Figure.text for incorrectly typesetting integers when mixed with floating-point numbers.

This PR adopts the latter one to fix the bug. An incorrect baseline image is also updated.

@seisman seisman added bug Something isn't working needs review This PR has higher priority and needs review. labels Oct 8, 2024
@seisman seisman added this to the 0.14.0 milestone Oct 8, 2024
@github-actions
Copy link
Contributor

github-actions bot commented Oct 8, 2024

Summary of changed images

This is an auto-generated report of images that have changed on the DVC remote

Status Path
modified pygmt/tests/baseline/test_text_nonstr_text.png

Image diff(s)

Added images

Modified images

Path Old New
test_text_nonstr_text.png

Report last updated at commit 8864945

@seisman seisman changed the title Figure.text: Fix the typesetting of non-string text Figure.text: Fix typesetting of integers when mixed with floating-point values Oct 9, 2024
@michaelgrund michaelgrund added final review call This PR requires final review and approval from a second reviewer and removed needs review This PR has higher priority and needs review. labels Oct 9, 2024
Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

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

Mark as breaking? Ideally, users would have figured out to use text=["1", "2.0", "3.14", "4."] instead of passing in integers, if they wanted a specific number of decimal points printed.

@seisman seisman changed the title Figure.text: Fix typesetting of integers when mixed with floating-point values **Breaking**: Figure.text: Fix typesetting of integers when mixed with floating-point values Oct 9, 2024
@seisman seisman changed the title **Breaking**: Figure.text: Fix typesetting of integers when mixed with floating-point values Figure.text: Fix typesetting of integers when mixed with floating-point values Oct 9, 2024
@seisman seisman changed the title Figure.text: Fix typesetting of integers when mixed with floating-point values **Breaking**: Figure.text: Fix typesetting of integers when mixed with floating-point values Oct 9, 2024
@seisman seisman merged commit 130d0cb into main Oct 9, 2024
@seisman seisman deleted the text/nonstr branch October 9, 2024 07:12
@seisman seisman removed the final review call This PR requires final review and approval from a second reviewer label Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants