-
Notifications
You must be signed in to change notification settings - Fork 367
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
Feature: add etopo plotting to stock_img #1747
base: main
Are you sure you want to change the base?
Conversation
Currently the PR is showing an empty etopo1.jpg committed? Any reason that needs to be included? |
The file etopo1.jpg is not empty. its size is 1.28M (64 dpi). it is the picture of the bathymetry data to plot. I took it from Basemao repo, because the original file downloaded from NOAA is about 2M (96 dpi). This file definitely will increase the size of cartopy. Alternatively, we can download it on the fly from NOAA website. Please let me know your opinion. |
I think grabbing it from the source is better whenever possible, that way we get any updates they have. It also looks like there are multiple "versions" of etopo, focusing on ice, or other features too? It looks like the original data is 300+MB! I didn't see the downsampled version immediately on NOAA's site, do you have a link to that? |
When you click on the thumbnail on https://www.ngdc.noaa.gov/mgg/global/, you will get to the downsampled version of etopo1: https://www.ngdc.noaa.gov/mgg/image/color_etopo1_ice_low.jpg. Yes, there are several versions of this dataset, depending on using the ice top or the ice bottom for altitude. The above link should suffice for our purpose. If we choose to grab it on the fly, its size is about 2M. |
977a75d
to
8a22d6f
Compare
8a22d6f
to
5d388ba
Compare
Now I have modified the code to download the etopo1 image from NOAA website, and delete the etopo1.jpg file from the commit. |
8a0abd2
to
8617bc2
Compare
8617bc2
to
fcfa10b
Compare
Can you rebase your commits to "squash" together the original commit adding the file and the one deleting it? Otherwise, there will be a copy of the 1.3MB file sitting in the history for this repo. |
fcfa10b
to
140f228
Compare
Thanks. I squashed all the commits into one. The test failure message below seems not to be related with this PR.
|
140f228
to
f2ea4fe
Compare
this PR appears to be reviwed and have passing tests, but there are conflicts between it and master @smartlixx please may i suggest that you i would hope that reviewers would then be able to re-assess and potentially merge this onto master @dopplershift @QuLogic is this reasonable from your points of view? |
f2ea4fe
to
d9866d4
Compare
d9866d4
to
5779145
Compare
55b5595
to
a867704
Compare
Yes, it is done. Please review and let me know your thoughts. @QuLogic @dopplershift |
a867704
to
1d35964
Compare
1d35964
to
3af8838
Compare
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.
Looks good to me, just one minor suggestion.
As a side comment/follow-up PR, would this make sense to have as a RasterSource feature artist? So, make it usable outside of the stock_img function, and have stock_img add the RasterSource features instead.
3af8838
to
16d7d95
Compare
772a697
to
c88e2a4
Compare
c88e2a4
to
dcc90b5
Compare
Thanks for your comments. I have updated the PR to incorporate the new changes since my last commit, and the PR has passed all the test. Hopefully it can be accepted (quite a long time has passed). Or if it is OK, I can also combine this PR with #2230.
I can see there is a cartopy/lib/cartopy/io/__init__.py Lines 310 to 365 in a6c0835
This may be a good starting point, but it's beyond my hands at this moment. |
Hi, I’m just checking on the status of this one. It looks like it was ready to go but unfortunately now has conflicts with the main branch again. @smartlixx do you have the bandwidth to rebase this again? |
|
Rationale
Close #1734
Implications
None as far as I can see.
Example
The output is