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

Replace the use of stamen tiles in the repo #451

Merged
merged 7 commits into from
Nov 29, 2024
Merged

Replace the use of stamen tiles in the repo #451

merged 7 commits into from
Nov 29, 2024

Conversation

Azaya89
Copy link
Collaborator

@Azaya89 Azaya89 commented Nov 21, 2024

The use of stamen tiles in the census example will be done under #430

@Azaya89 Azaya89 requested a review from hoxbro November 21, 2024 16:56
@Azaya89 Azaya89 self-assigned this Nov 21, 2024
Copy link
Contributor

Your changes were successfully integrated in the dev site, make sure to review the pages of the projects you touched before merging this PR.

@maximlt
Copy link
Contributor

maximlt commented Nov 21, 2024

It'd be nice if you could post screenshots of the new plots so we can see if the basemap colors don't confict too much with the data colors.

@Azaya89
Copy link
Collaborator Author

Azaya89 commented Nov 22, 2024

Landsat Old

landsat_old

Landsat new

landsat_new

Ml_annotator old

ml_old

Ml_annotator new

ml_new

Ship traffic old

ship_old

Ship traffic new

ship_new

@maximlt
Copy link
Contributor

maximlt commented Nov 22, 2024

Thanks @Azaya89! The comparison with the old maps doesn't really apply as the Stamen maps can no longer be loaded. You can visit for example the ML Annotator example and verify in your browser that the requests made to get the Stamen tiles fail. This example seems to have an overlay of tiles (ESRI * Stamen).

image

Copy link
Contributor

Your changes were successfully integrated in the dev site, make sure to review the pages of the projects you touched before merging this PR.

@Azaya89
Copy link
Collaborator Author

Azaya89 commented Nov 27, 2024

Datashader dashboard now

ds_dashboard

Ship traffic now

ship_t updated

ML_annotators now

image

When you zoom in, the labels also update dynamically.
@maximlt

Copy link
Contributor

Your changes were successfully integrated in the dev site, make sure to review the pages of the projects you touched before merging this PR.

Copy link
Contributor

@maximlt maximlt left a comment

Choose a reason for hiding this comment

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

Nice good progress :) !

I can see Census also use StamenLabels in two places, do you plan to update it too?
image

landsat/landsat.ipynb Outdated Show resolved Hide resolved
ship_traffic/ship_traffic.ipynb Show resolved Hide resolved
@@ -509,7 +514,7 @@
"sopts = dict(start=0, end=1, sizing_mode='stretch_width')\n",
"map_opacity = pn.widgets.FloatSlider(value=0.7, name=\"Map opacity\", **sopts)\n",
"data_opacity = pn.widgets.FloatSlider(value=1.0, name=\"Data opacity\", **sopts)\n",
"label_opacity = pn.widgets.FloatSlider(value=0.9, name=\"Label opacity\", **sopts)\n",
"label_opacity = pn.widgets.FloatSlider(value=0.3, name=\"Label opacity\", **sopts)\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

The labels are a bit hard to read on this map. Is it because they're too transparent?
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. Maybe the CartoDB.VoyagerOnlyLabels would show up better? I haven't tried it myself.

Also, what's up with the data rendering resolution? :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, what's up with the data rendering resolution? :-)

It's a screenshot from the website after zooming in :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The labels are a bit hard to read on this map. Is it because they're too transparent?

Yes, it's the map opacity. I've put the default back to 0.7 now

PositronOnly

ship_t_pos7

VoyagerOnly

ship_t_VO7

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't spot a difference between those two; are you sure they are different?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm sure. I too, can't see any visual difference.

Copy link
Contributor

Choose a reason for hiding this comment

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

Relocking can have some unwanted consequences, specially for projects like this that are stuck in the past. Can you make sure the notebook run fine locally please? (it might pass the tests fine but still be somewhat in a broken state).

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remind me why that project is stuck in the past? IIRC it's a quite simple project that I'd think was easy to update, but I don't know the constraints here.

Copy link
Contributor

Choose a reason for hiding this comment

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

The NG SDG grant wasn't enough funding to update all the examples so we haven't touch them all, like this one. PRs are welcome!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can you make sure the notebook run fine locally please? (it might pass the tests fine but still be somewhat in a broken state).

It ran locally fine before I pushed the commit. I usually do all the necessary pre-checks after re-locking to make sure nothing has broken.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can you remind me why that project is stuck in the past? IIRC it's a quite simple project that I'd think was easy to update, but I don't know the constraints here.

I can open separate PRs for updating the example, but I think we should finish up the tiles replacements here first?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make sure the notebook run fine locally please? (it might pass the tests fine but still be somewhat in a broken state).

It ran locally fine before I pushed the commit. I usually do all the necessary pre-checks after re-locking to make sure nothing has broken.

Nice, perfect! 👍

Copy link
Contributor

@maximlt maximlt Nov 28, 2024

Choose a reason for hiding this comment

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

Can you remind me why that project is stuck in the past? IIRC it's a quite simple project that I'd think was easy to update, but I don't know the constraints here.

I can open separate PRs for updating the example, but I think we should finish up the tiles replacements here first?

Updating the examples is an endless business :) datashader_dashboard was assigned priority 3 on our NF SDG sheet (from 1 to 6) and we didn't have enough time to get there (we updated almost half of them, not bad!). It's also special as @jbednar has an old PR that should be incorporated (#67).

My take is:

After that, let's take a breather and later re-assign priorities. We're a much larger team compared to when I joined, others could get involved more to help maintain the examples.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sounds good to me!

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see. By "stuck in the past" I thought you meant something was keeping it there other than the length of our to-do list. That's fine!

@Azaya89
Copy link
Collaborator Author

Azaya89 commented Nov 28, 2024

Nice good progress :) !

I can see Census also use StamenLabels in two places, do you plan to update it too?

Sure.

Copy link
Contributor

Your changes were successfully integrated in the dev site, make sure to review the pages of the projects you touched before merging this PR.

@Azaya89 Azaya89 requested a review from maximlt November 29, 2024 14:46
Copy link
Contributor

Your changes were successfully integrated in the dev site, make sure to review the pages of the projects you touched before merging this PR.

Copy link
Contributor

Your changes were successfully integrated in the dev site, make sure to review the pages of the projects you touched before merging this PR.

Copy link
Contributor

@maximlt maximlt left a comment

Choose a reason for hiding this comment

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

LGTM!

@maximlt maximlt merged commit 83352b0 into main Nov 29, 2024
21 checks passed
@jbednar
Copy link
Contributor

jbednar commented Nov 29, 2024

Thanks for fixing this problem!

If we're going to use the PositronOnly labels throughout our examples, should we add them to HoloViews/GeoViews tile sources so that we don't have to "import xyzservices.providers as xyz" and have that dependency? Fine if not, but if others agree, please file an issue to move in that direction in subsequent releases.

@jbednar jbednar deleted the patch-stamen292 branch November 29, 2024 19:41
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.

Replace broken Stamen tiles
3 participants