GenericMappingTools / remote-datasets

Documentation for remote datasets on the GMT server
https://www.generic-mapping-tools.org/remote-datasets/
5 stars 6 forks source link

The asterisk in the resolution codes is misleading #99

Closed KeranLi closed 3 months ago

KeranLi commented 10 months ago

Description of the problem

Hello guys! When I use grid = pygmt.datasets.load_earth_relief(resolution="06m", region=[70, 140, 0, 55]) to set a map of map, I found the provided resolution code can not be used.

Minimal Complete Verifiable Example

For example, when I set the resolution as "03m*", as codes as :

grid = pygmt.datasets.load_earth_relief(resolution="03m*", region=[70, 140, 0, 55])
fig = pygmt.Figure()
fig.grdimage(grid=grid, projection="M15c", frame="a", cmap="geo")
fig.colorbar(frame=["a1000", "x+lElevation", "y+lm"])
fig.show()

Full error message

---------------------------------------------------------------------------
GMTInvalidInput                           Traceback (most recent call last)
Cell In[21], line 1
----> 1 grid = pygmt.datasets.load_earth_relief(resolution="03m*", region=[70, 140, 0, 55])
      2 fig = pygmt.Figure()
      3 fig.grdimage(grid=grid, projection="M15c", frame="a", cmap="geo")

File f:\Anaconda3\envs\GeoTable\lib\site-packages\pygmt\helpers\decorators.py:738, in kwargs_to_strings..converter..new_module(*args, **kwargs)
    736             kwargs[arg] = separators[fmt].join(f"{item}" for item in value)
    737 # Execute the original function and return its output
--> 738 return module_func(*args, **kwargs)

File f:\Anaconda3\envs\GeoTable\lib\site-packages\pygmt\datasets\earth_relief.py:159, in load_earth_relief(resolution, region, registration, data_source, use_srtm)
    156 else:
    157     dataset_prefix = earth_relief_sources[data_source]
--> 159 grid = _load_remote_dataset(
    160     dataset_name="earth_relief",
    161     dataset_prefix=dataset_prefix,
    162     resolution=resolution,
    163     region=region,
    164     registration=registration,
    165 )
    166 return grid

File f:\Anaconda3\envs\GeoTable\lib\site-packages\pygmt\helpers\decorators.py:738, in kwargs_to_strings..converter..new_module(*args, **kwargs)
...
    279 # check registration
    280 if registration is None:
    281     # use gridline registration unless only pixel registration is available

GMTInvalidInput: Invalid resolution '03m*'.

System information

PyGMT information:
  version: v0.10.0
System information:
  python: 3.10.13 | packaged by Anaconda, Inc. | (main, Sep 11 2023, 13:24:38) [MSC v.1916 64 bit (AMD64)]
  executable: f:\Anaconda3\envs\GeoTable\python.exe
  machine: Windows-10-10.0.19041-SP0
Dependency information:
  numpy: 1.26.1
  pandas: 2.1.1
  xarray: 2023.10.1
  netCDF4: 1.6.5
  packaging: 23.2
  contextily: None
  geopandas: None
  IPython: 8.17.2
  rioxarray: None
  ghostscript: 10.02.0
GMT library information:
  binary version: 6.4.0
  cores: 12
  grid layout: rows
  image layout: 
  library path: F:/Anaconda3/envs/GeoTable/Library/bin/gmt.dll
  padding: 2
  plugin dir: F:/Anaconda3/envs/GeoTable/Library/bin/gmt_plugins
  share dir: F:/Anaconda3/envs/GeoTable/Library/share/gmt
  version: 6.4.0
welcome[bot] commented 10 months ago

šŸ‘‹ Thanks for opening your first issue here! Please make sure you filled out the template with as much detail as possible. You might also want to take a look at our contributing guidelines and code of conduct.

seisman commented 10 months ago

It should be 03m, not 03m*.

KeranLi commented 10 months ago

Well, it works! Thanks a lot! The table in docs is confusing.

image

seisman commented 10 months ago

At the top of the table, it says "An asterisk denotes tiled datasets", but I agree that it's a little misleading.

Transferring this issue report to https://github.com/GenericMappingTools/remote-datasets

seisman commented 10 months ago

Perhaps we should have another column (after the size column) to state if a grid is tiled or not? @PaulWessel @Esteban82 Thoughts?

KeranLi commented 10 months ago

At the top of the table, it says "An asterisk denotes tiled datasets", but I agree that it's a little misleading.

Transferring this issue report to https://github.com/GenericMappingTools/remote-datasets

Sorry, I did not notice this.

anbj commented 10 months ago

Perhaps we should have another column (after the size column) to state if a grid is tiled or not? @PaulWessel @Esteban82 Thoughts?

I agree - this is a better solution.

Still, I think what it says now is kind of OK. But then again, I'm the one who wanted the asterisk to indicate tiled datasets. "Clear as mud".

PaulWessel commented 10 months ago

I guess to help people who cannot read it would be simpler to have a separate column. I guess I am not sure what this information will do for people other than confuse? We implemented this system so it would be transparent and even things like @earth_relief by itself works. Who needs the under-the-hood technical stuff which can be explained in the repo readme?

anbj commented 10 months ago

My personal reason for doing this was to - well - simply have a fast way to check if a dataset was tiled or not. (Remembering which are tiled, and which are not, is not an option). When designing a map, I didnt want to use tiled datasets.

seisman commented 10 months ago

I guess I am not sure what this information will do for people other than confuse?

It's useful for the PyGMT team when we provide functions to load the dataset into xarray.DataArray object. Otherwise, we have to go to the data server and check if a grid is stored in a single file or in a directory.

PaulWessel commented 10 months ago

OK, fine with me as long as I dont have to do this! :-)

anbj commented 10 months ago

I can to it. Keep as is or make new column?

yvonnefroehlich commented 10 months ago

Hm. From a technical perspective, the table is fine as is, as the * is explained in the caption. But, I feel it can be misleading in different ways. When looking at this table for the first time before reading the documentation, I interpreted the * as a wildcard. So probably a separate column would be clearer.

anbj commented 10 months ago

So something like this?

Screenshot from 2023-11-03 13-52-31

yvonnefroehlich commented 10 months ago

So something like this?

Yes, I feel a / this separate column makes it easier / clearer for new users.

anbj commented 10 months ago

Yes, agree. I'll edit the others and make them like the none above.

joa-quim commented 10 months ago

Sorry, I clearly prefer a x over a /. That's how it is used in math.

anbj commented 10 months ago

I'm lost. What does / and x mean?

joa-quim commented 10 months ago

From what I understood, Yvonne was proposing to replace 360 x 180 by 360/180. But maybe it was me-misunderstanding triggered by the word columns

yvonnefroehlich commented 10 months ago

Oh, I am very sorry for the confusion caused šŸ™ˆ! Here, the / means simply or: Yes, I feel a or this separate column makes it easier or clearer for new users.

yvonnefroehlich commented 10 months ago

Sorry, I clearly prefer a x over a /. That's how it is used in math.

I am totaly fine with x šŸ™‚.

PaulWessel commented 10 months ago

No proposal to replace WxH with W/H, just adding the tiled/no tiled comments for reasons I dont fully understand but I dont care that much.

joa-quim commented 10 months ago

OK, but than (because without an example I'm still lost) what's best (and shorter) than what Andreas showed us above? Just Yes or No in that last column.

I don't understand the need either, but table looks better for example as support to explain why you don't want to do a gmtwhich on a tilled dataset.

PaulWessel commented 10 months ago

Maybe better to add a paragraph on the details page and explain why there are tiled and untiled versions of things?

seisman commented 10 months ago

Maybe better to add a paragraph on the details page and explain why there are tiled and untiled versions of things?

This page already explains why we do tiles:

To improve responsiveness, the larger files (i.e., currently for node spacings 05m and smaller) have been split into smaller tiles.

PaulWessel commented 10 months ago

You're right. All the things anyone who want or need to know is there. Not sure if we need that table column just because some rookie got confused. Maybe the original sin was to ad the asterisk to try to explain which was tiled even though the whole point of tiling and reassembly under the hood was so users should not need to know anything about this.

seisman commented 3 months ago

Perhaps we can remove the asterisk, add a note about which resolutions are tiled below the table and link to this page (https://docs.generic-mapping-tools.org/dev/datasets/remote-data.html) for reasons why we have tiled and untiled grids.

Esteban82 commented 3 months ago

Yes, I think it's a good idea.

seisman commented 3 months ago

@anbj Are you still willing to work on this issue?

anbj commented 3 months ago

@seisman Sure. A bit busy now, but I can do it when time permits.

So for each dataset, 1)remove asterisk, 2)add a note which resolutions are tiled and 3) link to remote-data.html?

seisman commented 3 months ago

Yes, also need to remove the "An asterisk denotes tiled datasets" note above tables.

seisman commented 3 months ago

Closed by #120.