Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Figure.text: Support non-ASCII characters in the 'text' parameter #2638

Merged
merged 7 commits into from
Aug 25, 2023

Conversation

seisman
Copy link
Member

@seisman seisman commented Aug 21, 2023

Description of proposed changes

Address #2204 following #2584.

Reminders

  • Run make format and make check to make sure the code follows the style guide.
  • Add tests for new features or tests that would have caught the bug that you're fixing.
  • Add new public functions/methods/classes to doc/api/index.rst.
  • Write detailed docstrings for all functions/methods.
  • If wrapping a new module, open a 'Wrap new GMT module' issue and submit reasonably-sized PRs.
  • If adding new functionality, add an example to docstrings or tutorials.
  • Use underscores (not hyphens) in names of Python files and directories.

Slash Commands

You can write slash commands (/command) in the first line of a comment to perform
specific operations. Supported slash commands are:

  • /format: automatically format and lint the code
  • /test-gmt-dev: run full tests on the latest GMT development version

@seisman seisman added the feature Brand new feature label Aug 21, 2023
@seisman seisman added this to the 0.10.0 milestone Aug 21, 2023
@seisman
Copy link
Member Author

seisman commented Aug 21, 2023

Here is the baseline image generated by the new test:
test_text_nonascii

I'm having some technical difficulties when running dvc push so can't push it to DAGsHub.

@seisman seisman added the needs review This PR has higher priority and needs review. label Aug 22, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 22, 2023

Summary of changed images

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

Status Path
added pygmt/tests/baseline/test_text_nonascii.png

Image diff(s)

Added images

  • test_text_nonascii.png

Modified images

Path Old New

Report last updated at commit 05f2299

@seisman
Copy link
Member Author

seisman commented Aug 22, 2023

I'm having some technical difficulties when running dvc push so can't push it to DAGsHub.

Suddenly it works for me. It may be some network issues yesterday.

@seisman seisman 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 Aug 22, 2023
pygmt/src/text.py Outdated Show resolved Hide resolved
Co-authored-by: Yvonne Fröhlich <[email protected]>
@weiji14 weiji14 mentioned this pull request Aug 25, 2023
35 tasks
@@ -223,7 +230,9 @@ def text_(

# Append text at last column. Text must be passed in as str type.
if kind == "vectors":
extra_arrays.append(np.atleast_1d(text).astype(str))
extra_arrays.append(
list(map(non_ascii_to_octal, np.atleast_1d(text).astype(str)))
Copy link
Member

Choose a reason for hiding this comment

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

This would be a list of numpy arrays, instead of being just a numpy array? Is there a way to keep it as a numpy array?

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to np.vectorize in 85a0926.

Comment on lines 383 to 385
fig.text(position="TL", text="position-text:°α")
fig.text(x=1, y=1, text="xytext:°α")
fig.text(x=[5, 5], y=[3, 5], text=["xytext1:°α", "xytext2:°α"])
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in adf6eda.

@seisman
Copy link
Member Author

seisman commented Aug 25, 2023

The new test fails, because I generated the baseline image using gs 9.56.1 but our CI is still using gs 9.54, but why can't the CI use gs 9.56.1?

@weiji14
Copy link
Member

weiji14 commented Aug 25, 2023

The new test fails, because I generated the baseline image using gs 9.56.1 but our CI is still using gs 9.54, but why can't the CI use gs 9.56.1?

Ghostscript 9.56.1 was just released 3 days ago on conda-forge - https://anaconda.org/conda-forge/ghostscript/files?version=9.56.1. I can also install 9.56.1 on my system, so not sure why CI still uses gs 9.54, let me check the dependency graph.

@weiji14
Copy link
Member

weiji14 commented Aug 25, 2023

Actually, maybe we don't want gs 9.56.1 for now 😅 I just ran make test locally with gs 9.56.1 and got 180 failed unit tests, pretty much all due to tiny image differences...

@seisman
Copy link
Member Author

seisman commented Aug 25, 2023

Actually, maybe we don't want gs 9.56.1 for now 😅 I just ran make test locally with gs 9.56.1 and got 193 failed unit tests, pretty much all due to tiny image differences...

OK. I think we can use gs 9.54 now.

FYI, gs 10.02.0 is likely the next gs version that doesn't have serious bugs (GenericMappingTools/gmt#7336 (comment)) and it's likely to be released soon (rc1 is already been released). I think we should only update these baseline images when gs 10.02.0 is out.

@weiji14
Copy link
Member

weiji14 commented Aug 25, 2023

Actually, maybe we don't want gs 9.56.1 for now 😅 I just ran make test locally with gs 9.56.1 and got 193 failed unit tests, pretty much all due to tiny image differences...

OK. I think we can use gs 9.54 now.

FYI, gs 10.02.0 is likely the next gs version that doesn't have serious bugs (GenericMappingTools/gmt#7336 (comment)) and it's likely to be released soon (rc1 is already been released). I think we should only update these baseline images when gs 10.02.0 is out.

Sounds good, better to update the baseline images once for gs 10.02 rather than twice, we can track the ghostscript 10.0 release on conda-forge at conda-forge/ghostscript-feedstock#26. Just realized that you put out the gs 9.56.1 package 😆

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.

Looks good to me. The 16 failing tests on CI will be fixed in #2636, so can safely ignore.

@seisman seisman merged commit 604d077 into main Aug 25, 2023
@seisman seisman deleted the text-non-ascii branch August 25, 2023 02:53
@seisman seisman removed the final review call This PR requires final review and approval from a second reviewer label Aug 25, 2023
weiji14 added a commit that referenced this pull request Sep 2, 2023
Tenth minor release of PyGMT.

* Add changelog entry to version switcher
* Update compatibility table
* Update citation
* Add draft changelog
* Add full names of contributors to changelog
* Add two highlight bullet points
* Combine non-ASCII character PRs #2638 and #2584 into one highlight point
* Swap author positions for Dongdong and Leo
* Change release date to 20230902
* Move Yvonne up a few spots

---------

Co-authored-by: Yvonne Fröhlich <[email protected]>
Co-authored-by: Dongdong Tian <[email protected]>
Co-authored-by: Michael Grund <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Brand new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants