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

Add a function basemap to get a static image from Tyler, and some converts for image #91

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

asinghvi17
Copy link
Member

This file provides the ability to get static base maps from Tyler.

Its main entry point is the basemap function, which returns a tuple
(x, y, z) of the image data (z) and its axes (x and y). The image
is in a web-mercator CRS, we need to figure out a way to also get eg WGS84 tiles.

This file also contains definitions for convert_arguments that make
the following syntax "just work":

image(TileProviders.Google(), Rect2f(-0.0921, 51.5, 0.04, 0.025), (1000, 1000); axis= (; aspect = DataAspect()))

You do still have to provide the extent and image size, but this is substantially better than nothing.

@asinghvi17
Copy link
Member Author

asinghvi17 commented Jul 9, 2024

(This doesn't work now but will work with #90, since that imports FileIO)

@rafaqz
Copy link
Collaborator

rafaqz commented Jul 9, 2024

Is there a reason basemap isn't in MapTiles.jl ?

Would let other packages skip the Makie dep

@asinghvi17
Copy link
Member Author

HTTP.jl and ImageIO.jl are required to stitch the tiles together, but if those can go in MapTiles I'm fine to upstream this

@rafaqz
Copy link
Collaborator

rafaqz commented Jul 9, 2024

Ahh right. Might be worth it for Plots.jl uses

@asinghvi17
Copy link
Member Author

This will have to be reimplemented based off #95

@asinghvi17 asinghvi17 marked this pull request as draft August 3, 2024 11:12
@asinghvi17 asinghvi17 self-assigned this Sep 26, 2024
@asinghvi17 asinghvi17 marked this pull request as ready for review September 26, 2024 14:12
@asinghvi17 asinghvi17 requested review from SimonDanisch, rafaqz and lazarusA and removed request for SimonDanisch and rafaqz September 26, 2024 14:12
@asinghvi17 asinghvi17 requested review from SimonDanisch, rafaqz and lazarusA and removed request for lazarusA and SimonDanisch September 26, 2024 14:12
@lazarusA
Copy link
Collaborator

lazarusA commented Sep 26, 2024

please add these lines at the end of our .css file

/* Component: Docstring Custom Block */

.jldocstring.custom-block {
  border: 1px solid var(--vp-c-gray-2);
  color: var(--vp-c-text-1)
}

.jldocstring.custom-block summary {
  font-weight: 700;
  cursor: pointer;
  user-select: none;
  margin: 0 0 8px;
}
Screenshot 2024-09-26 at 18 30 35

Edit: Also, it looks like your examples are not working.

Copy link
Collaborator

@rafaqz rafaqz left a comment

Choose a reason for hiding this comment

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

This is good, but it kinda seems to me like basemap should return a Raster so it's one self contained object with all the metadata needed to plot in GeoMakie etc

Of course, dependencies etc make that more annoying

src/basemap.jl Outdated Show resolved Hide resolved
````@example nasagibs
meshimage(
xs, ys, img;
source = "+proj=webmerc", # REMEMBER: `img` is always in Web Mercator...
Copy link
Collaborator

Choose a reason for hiding this comment

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

would be nice if we kept crs and lookups attached - I think there is a package that does that ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

That's true :D - but then we'd have to add that as a dependency to Tyler as well.

The other alternative is to add ImageIO and HTTP dependencies to MapTiles or TileProviders, and have them prompt individually. But I don't think we want that...

Copy link
Collaborator

@rafaqz rafaqz Sep 26, 2024

Choose a reason for hiding this comment

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

It would be nice to have a method of basemap that returned a Raster at least, in an extension here or in Rasters.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Question is how to trigger it. We could just pass the Raster type as the first argument?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would go the other way and create a constructor for Raster(::TileProvider, extent; size, res, ...).

Copy link
Member Author

Choose a reason for hiding this comment

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

This way it's at least a similar interface to what RasterDataSources has, for example

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, just call basemap internally to get the data. A bit like RasterDataSources.jl syntax

Copy link
Collaborator

Choose a reason for hiding this comment

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

Whats weird about that is it would need using Tyler which needs Makie when it really just needs ImageIO and HTTP

Copy link
Member Author

@asinghvi17 asinghvi17 Sep 26, 2024

Choose a reason for hiding this comment

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

It's the ImageIO part that gives me the shivers, since it's a lot of heavy binary dependencies

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm more scared of the Makie precompile time for Plots.jl users

return basemap(provider, bbox, _size; min_zoom_level, max_zoom_level)
end

function basemap(provider::TileProviders.AbstractProvider, boundingbox::Union{Rect2{<: Real}, Extent}, size::Tuple{Int, Int}; min_zoom_level = 0, max_zoom_level = 16)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't size allow a NamedTuple here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh right thats handled above. Maybe make this _basemap then so its internal

Copy link
Member Author

Choose a reason for hiding this comment

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

That sounds complicated - should it? I don't see that anywhere else as well

Copy link
Member Author

Choose a reason for hiding this comment

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

Then you have to handle (; X, Y), (; Y, X), ...

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just read in _z_index docs that res should be (X=res, Y=res) so thought that was the idea with size/res here. Most of Rasters works like this too so it makes sense to me


## Keyword arguments

`size` and `res` are mutually exclusive, and you must provide one.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be good to mention that they are approximate

`size` and `res` are mutually exclusive, and you must provide one.

`min_zoom_level - 0` and `max_zoom_level = 16` are the minimum and maximum zoom levels to consider.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we also just let users specify z here?

Copy link
Member Author

Choose a reason for hiding this comment

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

As an alternative to res and size, yes

- Some function that returns the tile size for a given provider / dispatch form
=#
tile_widths = (256, 256)
tilegrid_size = tile_widths .* length.(tilegrid.grid.indices)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should warn here if this is too big. Probably people don't intend to get more than 100MB or so? and its pretty easy to accidentally ask for a 100GB

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that could be an annoying footgun if using res instead of size

@lazarusA
Copy link
Collaborator

This one also looks like a lot of work has already been put into it. How do we move forward with this, or decide not to?

@asinghvi17
Copy link
Member Author

I think we want to factor a bunch of stuff out of Tyler and put it in MapTiles....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants