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

handle numeric key_on values for Choropleth #1193

Merged
merged 7 commits into from Nov 17, 2022

Conversation

alessioarena
Copy link
Contributor

This addresses issue #1185 where geometries and data coming from GeoDataFrames
were not joined. This was caused by the index being casted to string
during the conversion to GeoJSON.

The new behaviour is the following:

  • try the join as it is
  • if index contained numeric value, cast to str and try again

This commit includes 3 new test cases where:

  • 1st and 2nd were failing under old behaviour
  • 3rd passes under both behaviours

This addresses a bug where geometries and data coming from GeoDataFrames
were not joined. This was caused by the index being casted to string
during the conversion to GeoJSON.

The new behaviour is the following:
- try the join as it is
- if index contained numeric value, cast to str and try again

This commit includes 3 new test cases where:
- 1st and 2nd were failing under old behaviour
- 3rd passes under both behaviours
folium/features.py Outdated Show resolved Hide resolved
tests/test_folium.py Outdated Show resolved Hide resolved
tests/test_folium.py Outdated Show resolved Hide resolved
tests/test_folium.py Outdated Show resolved Hide resolved
tests/test_folium.py Outdated Show resolved Hide resolved
tests/test_folium.py Outdated Show resolved Hide resolved
tests/test_folium.py Outdated Show resolved Hide resolved
tests/test_folium.py Outdated Show resolved Hide resolved
folium/features.py Outdated Show resolved Hide resolved
folium/features.py Outdated Show resolved Hide resolved
folium/features.py Outdated Show resolved Hide resolved
folium/features.py Outdated Show resolved Hide resolved
folium/features.py Outdated Show resolved Hide resolved
Copy link
Member

@Conengmo Conengmo left a comment

Choose a reason for hiding this comment

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

Hi @alessioarena,

Sorry for my late review. I've been doubting about whether we should implement this or not. My hesitation is because of the added complexity and the possible side effects I'm not seeing. Any thoughts on that?

But given that this will solve something failing in Choropleth silently I'm inclined to merge this. Could you address my 2 comments?

folium/features.py Outdated Show resolved Hide resolved
# This is a pd.Series
data = data.set_index(columns[0])[columns[1]]

# convert the data to dictionary
Copy link
Member

Choose a reason for hiding this comment

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

Nit: this comment is a bit superfluous

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted. I can push the change, but I'm keen to wait for the other discussion to be agreed upon and resolved

@Conengmo Conengmo linked an issue Nov 16, 2022 that may be closed by this pull request
Copy link
Member

@Conengmo Conengmo left a comment

Choose a reason for hiding this comment

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

The original logic was actually not working for my test case, in which the geojson feature id is an integer and the data has a string index. I couldn't really work out what was going on, so I opted to simplify the logic instead: if the original key doesn't fit, try again with a str or int, depending on the original type. I hope this should be a bit simpler to grok.

@Conengmo Conengmo added the ready PR is ready for merging label Nov 16, 2022
@Conengmo Conengmo merged commit 4b393d0 into python-visualization:main Nov 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready PR is ready for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Choropleth map not displaying colors
3 participants