Skip to content

Update error message and docstring in st.map #5792

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-kbregula merged 2 commits into from
Dec 8, 2022
Merged

Update error message and docstring in st.map #5792

sfc-gh-kbregula merged 2 commits into from
Dec 8, 2022
+55 −27

Conversation

@sfc-gh-kbregula
Copy link
Contributor

@sfc-gh-kbregula sfc-gh-kbregula commented Nov 30, 2022 •
edited by snehankekre
Loading

📚 Context

The docstring for the data parameter of st.map could be better formatted and should include all the possible valid values of column names. Additionally, the st.map code includes many if-else blocks that could use refactoring.

  • What kind of change does this PR introduce?

    • Bugfix
    • Feature
    • Refactoring
    • Other, please describe: Doc improvement request

🧠 Description of Changes

  • Improves formatting of docstring for the data parameter of st.map

  • Includes all the possible valid values of column names for the data parameter and also does so in the error message

  • Refactors st.map code to replace multiple if-else blocks with iterators

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

image

Current:

image

🧪 Testing Done

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

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-kbregula sfc-gh-kbregula added the security-assessment-completed Security assessment has been completed for PR label Nov 30, 2022
@sfc-gh-tszerszen sfc-gh-tszerszen self-requested a review November 30, 2022 12:04
sfc-gh-tszerszen
sfc-gh-tszerszen approved these changes Nov 30, 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 👍

@lukasmasuch lukasmasuch self-requested a review December 1, 2022 19:25
snehankekre
snehankekre suggested changes Dec 2, 2022
View reviewed changes
@snehankekre snehankekre self-requested a review December 2, 2022 09:19
@snehankekre
Copy link
Contributor

snehankekre commented Dec 2, 2022 •
edited
Loading

@sfc-gh-kbregula On 0b29d42, I linted with black to fix the failing CI tests.

I've also edited your PR description to include all the relevant information and context so there's a historical record of why these changes were made.

Could you also include a PR comment saying why this PR does not need new tests? Thanks! See the new Processes doc for more info on why to talk about tests.

@sfc-gh-kbregula
Copy link
Contributor Author

sfc-gh-kbregula commented Dec 2, 2022

Could you also include a PR comment saying why this PR does not need new tests?

This is a refactoring that doesn't change the behavior. Current tests already cover this case. I also changed the message, which was updated in the tests. I think, we don't need new tests.

lukasmasuch
lukasmasuch approved these changes Dec 2, 2022
View reviewed changes
Copy link
Collaborator

@lukasmasuch lukasmasuch left a comment

Choose a reason for hiding this comment

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

LGMT 👍 Since it is purely refactoring it does indeed not need a test. Unfortunately, in this specific case there does not seem to be any test that covers the different allowed values "lon", "longitude", "LON", "LONGITUDE". So, it would be awesome to extend the testing in this PR to cover these alternative allowed column names.

@sfc-gh-kbregula
Copy link
Contributor Author

sfc-gh-kbregula commented Dec 5, 2022

@lukasmasuch Update tests. Now we have better coverage.

@harahu
Copy link
Contributor

harahu commented Dec 6, 2022

The docs state that you should be able to pass in an numpy.ndarray. They don't have column names. How does that work?

@snehankekre
Copy link
Contributor

snehankekre commented Dec 7, 2022 •
edited
Loading

@harahu good question. That's likely a bug in the docstring.

If a numpy.ndarray is passed, it first gets converted to a pd.DataFrame:

streamlit/lib/streamlit/elements/map.py

Line 169 in a5e8141

data = type_util.convert_anything_to_df(data)

after which it always fails the check for the latitude or lat columns and an exception is raised:

streamlit/lib/streamlit/elements/map.py

Lines 180 to 184 in a5e8141

else:
raise StreamlitAPIException(
"Map data must contain a column named 'latitude' or 'lat'. "
f"Existing columns: {formmated_column_names}"
)

@sfc-gh-kbregula we should probably remove numpy.ndarray, from the docstring, right?

I'm assuming numpy.ndarray is an Iterable(?) If so, perhaps the the allowed types for the data arg are too broad?

@sfc-gh-kbregula
Copy link
Contributor Author

sfc-gh-kbregula commented Dec 8, 2022 •
edited
Loading

For the following code:

arr = np.array([[1, 2], [4, 5]], np.int32)
st.map(arr)

I got the following error:

streamlit.errors.StreamlitAPIException: Map data must contain a latitude column named: 'LAT', 'LATITUDE', 'lat', 'latitude'. Existing columns: 0, 1

It looks a bit like a bug to me, because I can imagine that the user enters data as a two-dimensional array. For now, I will create a ticket to discuss this problem and think about the solution.

@sfc-gh-kbregula sfc-gh-kbregula merged commit ef8bb02 into develop Dec 8, 2022
@sfc-gh-kbregula sfc-gh-kbregula mentioned this pull request Dec 8, 2022
Closed
5 tasks
@sfc-gh-kbregula
Copy link
Contributor Author

sfc-gh-kbregula commented Dec 8, 2022

Ticket: #5835

@vdonato vdonato deleted the error-message-st-map branch December 13, 2022 23:42
tconkling added a commit that referenced this pull request Dec 20, 2022
@tconkling
Merge branch 'develop' into feature/ReleaseCacheV2
981a619 
* develop:
  Change workers balancing logic for e2e tests to always cover all specs (#5865)
  Up version to 1.16.0 (#5852)
  Stop installing pip-tools (#5854)
  Add eslint no relative import paths plugin to pre commit hooks (#5839)
  Docs: move note to `experimental_allow_widgets` param description (#5843)
  Update element docstrings for colored text support (#5831)
  Add remark-directive plugin (Support for colored text in markdown) (#5774)
  Teach WebsocketConnection how to wait on an external auth token (#5728)
  Turn on streamlit theme for altair and plotly (#5796)
  Fix markdown headers spacing (#5829)
  Update error message and docstring in st.map (#5792)
  Remove unnecessarily methods from DeltaGenerator (#5824)
  Resolve vega-tooltip to later version (#5825)
  Easter Egg (#5817)
  Update Exception Layout to avoid overflow (#5700)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lukasmasuch lukasmasuch lukasmasuch approved these changes

+2 more reviewers

@snehankekre snehankekre snehankekre approved these changes

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

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

security-assessment-completed Security assessment has been completed for PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@sfc-gh-kbregula @snehankekre @harahu @lukasmasuch @sfc-gh-tszerszen