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.grdimage: Let the img_out parameter work as documented #2765

Closed
wants to merge 12 commits into from

Conversation

seisman
Copy link
Member

@seisman seisman commented Oct 24, 2023

Description of proposed changes

grdimage -A option works diffenrently for GMT CLI and external wrappers.

For GMT CLI, the syntax is like gmt grdimage ingrd.nc -Aimg_out.xxx.
For external wrappers, the syntax is like gmt grdimage ingrid.nc -A > img_out.xxx.

This PR makes img_out parameter working as documented, i.e., img_out="out.tiff" works.

Address #2599.

@seisman seisman added the bug Something isn't working label Oct 24, 2023
@seisman seisman added this to the 0.11.0 milestone Oct 24, 2023
@seisman seisman marked this pull request as draft October 24, 2023 16:29
@seisman seisman changed the title Figure.grdimage: Let the img_out parameter work as documented WIP: Figure.grdimage: Let the img_out parameter work as documented Oct 24, 2023
@seisman
Copy link
Member Author

seisman commented Oct 24, 2023

This PR works well for me locally on macOS, but I have no idea why it fails on CI.

@weiji14
Copy link
Member

weiji14 commented Oct 24, 2023

This PR works well for me locally on macOS, but I have no idea why it fails on CI.

Is this only for GMT 6.5? Doesn't work locally for me with GMT 6.4.0 on Linux.

@seisman
Copy link
Member Author

seisman commented Dec 11, 2023

Running the test_grdimage_img_out test alone works well for me:

pytest pygmt/tests/test_grdimage.py -k test_grdimage_img_out

but it causes crashes if running with the test_image test. Here is the command to reproduce the crash:

pytest pygmt/tests/test_grdimage.py pygmt/tests/test_image.py -k 'test_grdimage_img_out or test_image'

@seisman seisman changed the title WIP: Figure.grdimage: Let the img_out parameter work as documented Figure.grdimage: Let the img_out parameter work as documented Dec 11, 2023
@seisman seisman modified the milestones: 0.12.0, 0.11.0 Dec 11, 2023
@seisman seisman marked this pull request as ready for review December 11, 2023 13:07
@seisman seisman added the needs review This PR has higher priority and needs review. label Dec 11, 2023
@seisman
Copy link
Member Author

seisman commented Dec 12, 2023

As reported in #2599 (comment), now the img_out parameter works for raster image formats like png, jpg, tiff, but doesn't work for image formats that require a "driver".

@seisman seisman requested a review from a team December 17, 2023 05:57
@seisman
Copy link
Member Author

seisman commented Dec 20, 2023

Ping @GenericMappingTools/pygmt-maintainers

Please see #2599 for the related discussions, especially comments like #2599 (comment) and #2599 (comment).

The grdimage module usually writes PostScript codes to a hidden PostScript file, but with -A option, it produces a raster/vector image without writing any PS codes. So, it's a totally different workflow.

As mentioned in #2599 (comment), grdimage -A works like gdal_translate, but allows grid projection, shading and maybe more. So, the feature is still useful.

So, I propose to:

  1. Do NOT support -A option in the grdimage method
  2. Fully remove img_out parameter from the grdimage method
  3. If such a feature is requested by users, then we can add a new function (e.g., pygmt.grd2img) which wraps the grdimage -A option.

Thoughts?

@weiji14
Copy link
Member

weiji14 commented Dec 21, 2023

Fully remove img_out parameter from the grdimage method

Sound good. So we're removing the alias without a deprecation cycle right? Since it didn't really work before?

@seisman
Copy link
Member Author

seisman commented Dec 21, 2023

Fully remove img_out parameter from the grdimage method

Sound good. So we're removing the alias without a deprecation cycle right? Since it didn't really work before?

Yes, but I think it's better to raise a warning/error if -A/img_out is used.

@seisman seisman removed the needs review This PR has higher priority and needs review. label Dec 21, 2023
@seisman
Copy link
Member Author

seisman commented Dec 21, 2023

Superseded by #2907. Closing.

@seisman seisman closed this Dec 21, 2023
@seisman seisman deleted the grdimage/img_out branch December 21, 2023 13:09
@seisman seisman removed this from the 0.11.0 milestone Dec 21, 2023
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