-
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
Add function to load Black Marble dataset #3469
Conversation
Function to load the `@earth_night_` NASA Black Marble mosaic RGB images stored as GeoTIFF files.
# If rioxarray is installed, set the coordinate reference system | ||
if hasattr(image, "rio"): | ||
image = image.rio.write_crs(input_crs="OGC:CRS84") |
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.
This line is not covered by a unit test. Should I add one, or we can ignore it?
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.
It should be covered when rioxarray is installed, right?
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.
Or do we have to import rioxarray to register the rio accessor
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 equivalent line in load_blue_marble
is covered here in test_grdimage_image.py
which has an import rioxarray
at the top of the file:
pygmt/pygmt/tests/test_grdimage_image.py
Line 18 in 66f258a
xr_image = load_blue_marble(resolution="01d") |
We don't test load_black_marble
anywhere else except in test_datasets_earth_night.py
, and since rioxarray
is not imported in that file, this line isn't covered because there is no rio
accessor.
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.
Maybe we can cover this line in the test_grdcut_image.py
file at https://github.com/GenericMappingTools/pygmt/pull/3115/files#r1780370539?
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.
since
rioxarray
is not imported in that file, this line isn't covered because there is norio
accessor.
This is not ideal. So even if rioxarray is installed, the rio
accessor may still not exist because users don't import the rioxarray package.
I think we should add the following lines to this file (and any other files that try to access the rio
accessor):
pygmt/pygmt/datasets/tile_map.py
Lines 21 to 23 in 66f258a
with contextlib.suppress(ImportError): | |
# rioxarray is needed to register the rio accessor | |
import rioxarray # noqa: F401 |
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.
xref: #3323
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.
Ok, done at commit 66183ed. I've also applied this to pygmt/datasets/earth_day.py
, hopefully you don't mind.
Alos need to add |
Description of proposed changes
Function to load the
@earth_night_
NASA Black Marble mosaic RGB images stored as GeoTIFF files.Preview at https://pygmt-dev--3469.org.readthedocs.build/en/3469/api/generated/pygmt.datasets.load_black_marble.html
Example usage:
produces
Implementation is adapted from #2235.
Fixes #1442, supersedes #1457
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 command is:
/format
: automatically format and lint the code