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.subplot: Fix strange positioning issues after exiting subplot #2427

Merged
merged 7 commits into from
Mar 16, 2023

Conversation

seisman
Copy link
Member

@seisman seisman commented Mar 15, 2023

Description of proposed changes

Need to use separate sessions for subplot begin and subplot end. Otherwise, subplot end will use the "last session" (i.e., the lib variable created by other calls), which may cause strange behaviors like #2426.

After this PR, the PyGMT example in #2426 works as expected.

TODOs:

Fixes #

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 bug Something isn't working label Mar 15, 2023
@seisman seisman added this to the 0.9.0 milestone Mar 15, 2023
@seisman seisman marked this pull request as ready for review March 15, 2023 14:51
@github-actions
Copy link
Contributor

github-actions bot commented Mar 15, 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_subplot_outside_plotting_positioning.png

Image diff(s)

Added images

  • pygmt/tests/baseline/test_subplot_outside_plotting_positioning.png

Modified images

Path Old New

Report last updated at commit dc0fec4

@michaelgrund michaelgrund added the final review call This PR requires final review and approval from a second reviewer label Mar 15, 2023
pygmt/tests/test_subplot.py Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Does the plot need updating? The unit test is failing at https://github.com/GenericMappingTools/pygmt/actions/runs/4427893754/jobs/7766176078#step:11:704:

__________________ test_subplot_outside_plotting_positioning ___________________
Error: Image files did not match.
  RMS Value: 3.6267715100983793
  Expected:  
    /home/runner/work/pygmt/pygmt/tmp-test-dir-with-unique-name/results/pygmt.tests.test_subplot.test_subplot_outside_plotting_positioning/baseline.png
  Actual:    
    /home/runner/work/pygmt/pygmt/tmp-test-dir-with-unique-name/results/pygmt.tests.test_subplot.test_subplot_outside_plotting_positioning/result.png
  Difference:
    /home/runner/work/pygmt/pygmt/tmp-test-dir-with-unique-name/results/pygmt.tests.test_subplot.test_subplot_outside_plotting_positioning/result-failed-diff.png
  Tolerance: 
    2

Copy link
Member Author

Choose a reason for hiding this comment

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

Minor differences are likely due to different gs versions, but I'm using gs 9.54.0 from conda-forge on macOS and the test passes for me locally:
result-failed-diff

Copy link
Member

Choose a reason for hiding this comment

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

Are you using a dev version of GMT? Can you show your pygmt.show_versions()? This is the one from CI at https://github.com/GenericMappingTools/pygmt/actions/runs/4427893754/jobs/7766177139#step:11:16 which is also using ghostscript 9.54.0:

PyGMT information:
  version: v0.8.1.dev106+g52f04401
System information:
  python: 3.11.0 | packaged by conda-forge | (main, Jan 15 2023, 05:44:48) [Clang 14.0.6 ]
  executable: /Users/runner/miniconda3/envs/pygmt/bin/python
  machine: macOS-12.6.3-x86_64-i386-64bit
Dependency information:
  numpy: 1.24.2
  pandas: 1.5.3
  xarray: 2023.2.0
  netCDF4: 1.6.3
  packaging: 23.0
  contextily: 1.3.0
  geopandas: 0.12.2
  ghostscript: 9.54.0
GMT library information:
  binary version: 6.4.0
  cores: 3
  grid layout: rows
  image layout: 
  library path: /Users/runner/miniconda3/envs/pygmt/lib/libgmt.dylib
  padding: 2
  plugin dir: /Users/runner/miniconda3/envs/pygmt/lib/gmt/plugins
  share dir: /Users/runner/miniconda3/envs/pygmt/share/gmt
  version: 6.4.0

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you using a dev version of GMT?

aha, you're right. I'm using GMT master.

Copy link
Member

Choose a reason for hiding this comment

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

Great, now the GMT Dev Tests are failing on the main branch at https://github.com/GenericMappingTools/pygmt/actions/runs/4443039566/jobs/7799909484#step:17:718 🙃 Should we set an @pytest.mark.xfail?

Copy link
Member Author

Choose a reason for hiding this comment

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

What about increase the RMS diff threshold to 4.0 @pytest.mark.mpl_image_compare(tolerance=4.0)?

@seisman seisman removed the final review call This PR requires final review and approval from a second reviewer label Mar 16, 2023
@seisman seisman merged commit 598a5ab into main Mar 16, 2023
@seisman seisman deleted the fix-subplot-history branch March 16, 2023 04:53
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.

3 participants