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

Fix bug with different band dtype from load_tile_map by casting to uint8 #2393

Merged
merged 2 commits into from
Mar 4, 2023

Conversation

weiji14
Copy link
Member

@weiji14 weiji14 commented Mar 3, 2023

Description of proposed changes

The band list [0, 1, 2] was being converted to int32 on Windows + NumPy 1.24, but is int64 on other platforms. Casting the band coordinate explicitly to uint8 for consistency.

Patches bug in #2125 as reported in #2125 (comment)

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

The band list `[0, 1, 2]` was being converted to int32 on Windows + NumPy 1.24, but is int64 on other platforms. Casting the band coordinate explicitly to `uint8` for consistency.
@weiji14 weiji14 added the bug Something isn't working label Mar 3, 2023
@weiji14 weiji14 added this to the 0.9.0 milestone Mar 3, 2023
@weiji14 weiji14 self-assigned this Mar 3, 2023
Comment on lines 106 to 107
* y (y) float64 1.663e+05 1.663e+05 1.663e+05 ... 1.272e+05 ...
* x (x) float64 1.153e+07 1.153e+07 1.153e+07 ... 1.158e+07 ...
Copy link
Member Author

@weiji14 weiji14 Mar 3, 2023

Choose a reason for hiding this comment

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

From #2125 (comment), I remembered why I didn't print out the full y and x coordinates here. It's because that would make the docstring too long (>79 characters), so I added ... which is supported by https://docs.python.org/3/library/doctest.html#doctest.ELLIPSIS.

-      * y        (y) float64 1.663e+05 1.663e+05 1.663e+05 ... 1.272e+05 ...
-      * x        (x) float64 1.153e+07 1.153e+07 1.153e+07 ... 1.158e+07 ...
+      * y        (y) float64 1.663e+05 1.663e+05 1.663e+05 ... 1.272e+05 1.272e+05
+      * x        (x) float64 1.153e+07 1.153e+07 1.153e+07 ... 1.158e+07 1.158e+07

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps you need to add the # doctest: +ELLIPSIS directive after line 103.

Copy link
Member Author

@weiji14 weiji14 Mar 4, 2023

Choose a reason for hiding this comment

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

Perhaps you need to add the # doctest: +ELLIPSIS directive after line 103.

I thought so too, but using ... by itself seems to work? Like, the tests still all pass. I think it was just the int32/int64 diff that was the problem, but pytest still reports the full diff.

Anyways, I've changed the bounding box region in ea9e7dc and the y/x coordinates fit under 79 characters now, so doesn't really matter.

@weiji14
Copy link
Member Author

weiji14 commented Mar 3, 2023

All the tests pass now, but I just noticed at https://github.com/GenericMappingTools/pygmt/actions/runs/4327743401/jobs/7556724608#step:11:875 that the doctest is taking 6.76 seconds (the 2nd slowest test). Should we update the region and/or set a zoom level to return a smaller tile instead?

@seisman
Copy link
Member

seisman commented Mar 4, 2023

All the tests pass now, but I just noticed at https://github.com/GenericMappingTools/pygmt/actions/runs/4327743401/jobs/7556724608#step:11:875 that the doctest is taking 6.76 seconds (the 2nd slowest test). Should we update the region and/or set a zoom level to return a smaller tile instead?

Yes, we need a smaller tile.

Just plotting the Southern Hemisphere at zoom level 1.
@weiji14
Copy link
Member Author

weiji14 commented Mar 4, 2023

All the tests pass now, but I just noticed at https://github.com/GenericMappingTools/pygmt/actions/runs/4327743401/jobs/7556724608#step:11:875 that the doctest is taking 6.76 seconds (the 2nd slowest test). Should we update the region and/or set a zoom level to return a smaller tile instead?

Yes, we need a smaller tile.

Ok, I've modified the bounding box region and set things to zoom level 1 in ea9e7dc, which should return only 2 tiles of size 256x256. The doctest now takes 0.60 seconds according to https://github.com/GenericMappingTools/pygmt/actions/runs/4330035373/jobs/7560981253#step:11:1116

@weiji14 weiji14 merged commit 04924ef into main Mar 4, 2023
@weiji14 weiji14 deleted the patch/load_tile_map_xr_coords branch March 4, 2023 08:23
@weiji14 weiji14 added the skip-changelog Skip adding Pull Request to changelog label Mar 31, 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 skip-changelog Skip adding Pull Request to changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants