Skip to content

Display existing column names in st.map exception #5679

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.

Sign up for GitHub

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

Jump to bottom
Merged
sfc-gh-tszerszen merged 4 commits into from
Nov 17, 2022
Merged

Display existing column names in st.map exception #5679

sfc-gh-tszerszen merged 4 commits into from
Nov 17, 2022

Conversation

@sfc-gh-kbregula
Copy link
Contributor

@sfc-gh-kbregula sfc-gh-kbregula commented Nov 9, 2022 •
edited by sfc-gh-tszerszen
Loading

📚 Context

Thanks @sfc-gh-kbregula for noticing this, I talked to @jrieke and we can just use capital letters column names for st.map, this PR does it additionally to your improved error message.

Below is @sfc-gh-kbregula original message:

I started using our newest Snowpark integration and wrote the code below.

df = snowpark_session.sql("""
    SELECT * FROM
        (
            VALUES (1, 10), (2, 20), (3, 30), (4, 40)
        ) AS V1(lat, lot)
""")
st.map(df)

It turned out that the code above doesn't work because Snowpark returns all columns as uppercase. From the exception message, this was not easily discoverable, so I am adding the column names to make the message more user-friendly and actionable.

After merging this change, running the above code will result in the message below.

Map data must contain a column named 'latitude' or 'lat'. Existing columns: 'LAT', 'LAT'

Another issue is whether we should check column names ignoring case, but this will probably need some discussion, so I'm not changing it yet.

Please describe the project or issue background here

  • What kind of change does this PR introduce?

    • Bugfix
    • Feature
    • Refactoring
    • Other, please describe:

🧠 Description of Changes

  • Add bullet points summarizing your changes here

    • This is a breaking API change
    • This is a visible (user-facing) change

Revised:

Insert screenshot of your updated UI/code here

Current:

Insert screenshot of existing UI/code here

🧪 Testing Done

  • Screenshots included
  • Added/Updated unit tests
  • Added/Updated e2e tests

🌐 References

Does this depend on other work, documents, or tickets?

  • Issue: Closes #XXXX

Contribution License Agreement

By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

@sfc-gh-tszerszen sfc-gh-tszerszen force-pushed the sfc-gh-kbregula-exception-names branch from ad18430 to c7c8fc5 Compare November 17, 2022 11:37
@sfc-gh-tszerszen sfc-gh-tszerszen self-assigned this Nov 17, 2022
sfc-gh-tszerszen
sfc-gh-tszerszen approved these changes Nov 17, 2022
View reviewed changes
Copy link
Contributor

@sfc-gh-tszerszen sfc-gh-tszerszen left a comment

Choose a reason for hiding this comment

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

LGTM! 👍 Thanks for noticing this, I've updated the PR for st.map to work with capital letters LON, LAT etc.

@sfc-gh-tszerszen sfc-gh-tszerszen merged commit 4314217 into develop Nov 17, 2022
sfc-gh-kbregula
sfc-gh-kbregula commented Nov 17, 2022
View reviewed changes
lib/streamlit/elements/map.py
else:
raise StreamlitAPIException(
'Map data must contain a column named "latitude" or "lat".'
"Map data must contain a column named 'latitude' or 'lat'. "
Copy link
Contributor Author

@sfc-gh-kbregula sfc-gh-kbregula Nov 17, 2022

Choose a reason for hiding this comment

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

Should we also update the error message?

sfc-gh-kbregula
sfc-gh-kbregula commented Nov 17, 2022
View reviewed changes
lib/streamlit/elements/map.py
Comment on lines 172 to 180
if "lat" in data:
lat = "lat"
elif "latitude" in data:
lat = "latitude"
elif "LAT" in data:
lat = "LAT"
elif "LATITUDE" in data:
lat = "LATITUDE"
else:
Copy link
Contributor Author

@sfc-gh-kbregula sfc-gh-kbregula Nov 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We can simplify it a bit.

    lat_fields = ["lat", "latitude", "LAT", "LATITUDE"]
    lat = next((d for d in lat_fields if d in data), None)
    if not lat:

And then we can use the lat_fields variable with error messages.

@sfc-gh-kbregula
Copy link
Contributor Author

sfc-gh-kbregula commented Nov 17, 2022

Can you also update docstring of st.map function?
https://github.com/streamlit/streamlit/blob/c7c8fc5925e6882c4b6533146021892faeb67f52/lib/streamlit/elements/map.py#L100-101

@sfc-gh-tszerszen
Copy link
Contributor

sfc-gh-tszerszen commented Nov 17, 2022

@sfc-gh-kbregula good call out, we can do it in separate PR later on, this is merged so we got rid of the bug ASAP

tconkling added a commit to tconkling/streamlit that referenced this pull request Nov 18, 2022
@tconkling
Merge branch 'develop' into tim/CLICleanup
3c70e44 
* develop: (25 commits)
  Fix CORS acronym in docstring (streamlit#5727)
  Add integration tests for Snowpark (streamlit#5543)
  Release/1.15.0 (streamlit#5720)
  Add audit_frontend_dependencies script to CODEOWNERS (streamlit#5708)
  Add label_visibility option for st.checkbox (streamlit#5705)
  Display existing column names in st.map exception and make st.map work with capital letters (streamlit#5679)
  Plotly Customization (streamlit#5681)
  Turn off theme for now (streamlit#5701)
  Add all vendored code to `make notices` (streamlit#5704)
  Audit frontend licenses (streamlit#5664)
  Surround labels in quotes (streamlit#5703)
  Add info about voting on features (streamlit#5660)
  Update issue labeling scheme to adopt new standards (streamlit#5702)
  Cached media (audio+video) replay (streamlit#5695)
  Fix docstring line wrap (streamlit#5698)
  Use specialized assertion functions (streamlit#5680)
  Release 1.14.1 (streamlit#5693)
  Image replay in cached functions (streamlit#5675)
  Add docstrings for `experimental_allow_widgets` (streamlit#5670)
  Remove `st.write` from `CachedStFunctionWarning` (streamlit#5669)
  ...
@vdonato vdonato deleted the sfc-gh-kbregula-exception-names branch November 2, 2023 00:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jrieke jrieke Awaiting requested review from jrieke

@sfc-gh-lmasuch sfc-gh-lmasuch Awaiting requested review from sfc-gh-lmasuch

1 more reviewer

@sfc-gh-tszerszen sfc-gh-tszerszen sfc-gh-tszerszen approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@sfc-gh-tszerszen sfc-gh-tszerszen

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sfc-gh-kbregula @sfc-gh-tszerszen