-
Notifications
You must be signed in to change notification settings - Fork 224
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.savefig: Add the 'worldfile' parameter to write a companion world file for raster images #2766
Conversation
Co-authored-by: Yvonne Fröhlich <[email protected]>
pygmt/figure.py
Outdated
@@ -340,6 +356,13 @@ def savefig( | |||
kwargs["Qt"] = 2 | |||
kwargs["Qg"] = 2 | |||
|
|||
if worldfile: | |||
if ext in ["kml", "tiff"]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are worldfiles possible for other formats like pdf/bmp/eps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes for PDF and BMP.
Aslo yes for EPS, but for unknown reasons, the current codes doesn't produce a .esw
file correctly. Need some time to find out why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think I've ever heard of people create world files for PDF (pfw files?!), but ok then if it works. Have added another suggestion below to test that bmp/bpw files are created.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are worldfiles possible for other formats like pdf/bmp/eps?
For EPS, see an upstream issue report GenericMappingTools/gmt#7976
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See GenericMappingTools/gmt#7976, a world file is useless for vector images like EPS and PDF.
Co-authored-by: Wei Ji <[email protected]>
pygmt/figure.py
Outdated
@@ -340,6 +356,13 @@ def savefig( | |||
kwargs["Qt"] = 2 | |||
kwargs["Qg"] = 2 | |||
|
|||
if worldfile: | |||
if ext in ["kml", "tiff"]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think I've ever heard of people create world files for PDF (pfw files?!), but ok then if it works. Have added another suggestion below to test that bmp/bpw files are created.
54e72e3
to
a863132
Compare
@@ -292,6 +292,27 @@ def mock_psconvert(*args, **kwargs): # pylint: disable=unused-argument | |||
} | |||
|
|||
|
|||
def test_figure_savefig_worldfile(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was there a reason to switch from the parametrized tests to a single test in a863132? It'll be harder to see which formats/extensions fail, if e.g. the first one in a list like ['.bmp', '.jpg', ...] raises an AssertionError, because the others in the list won't be tested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main idea is to reducing the number of tests and also testing times. Comparing https://github.com/GenericMappingTools/pygmt/actions/runs/6677102075/job/18146781350?pr=2766 and https://github.com/GenericMappingTools/pygmt/actions/runs/6648648090/job/18065964154?pr=2766, the old way (using parametrize
) has 7 tests for worldfile, and the testing time is 0.5 * 5 + 0.25 * 2 = 3 seconds, while the new way (a single test) costs 1.5 s only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok, I see that we avoid having to call fig.basemap
multiple times, so it will be faster. All good then.
pygmt/figure.py
Outdated
but with different extension (e.g. tfw for tif). See | ||
https://en.wikipedia.org/wiki/World_file#Filename_extension | ||
for the convention of world file extensions. This parameter only | ||
works for raster image formats. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Geo)TIFF is a raster file format, but a worldfile won't be produced. Also, maybe best to just list out the supported extensions.
works for raster image formats. | |
works for non-georeferenced raster image formats (e.g. '.bmp', | |
'.jpg', '.png', '.ppm', '.tif') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In PR #2771 (commit 0c81847), we have revised the function description to separate the list of raster and vector formats (preview: https://www.pygmt.org/dev/api/generated/pygmt.Figure.savefig.html#pygmt.Figure.savefig).
So I prefer to not list the formats again and prefer to revise the docstring to
works for raster image formats (except GeoTIFF).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right, totally missed that. Your shorter suggestion sounds good.
Description of proposed changes
Address #2698 (comment).
Fixes #
Reminders
make format
andmake check
to make sure the code follows the style guide.doc/api/index.rst
.Slash Commands
You can write slash commands (
/command
) in the first line of a comment to performspecific operations. Supported slash commands are:
/format
: automatically format and lint the code/test-gmt-dev
: run full tests on the latest GMT development version