This repository has been archived by the owner on Dec 28, 2021. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Update geomaps to allow dataframes as input. #1187
Update geomaps to allow dataframes as input. #1187
Changes from 5 commits
78f2551
f22cc7c
d9f6444
27eb6d9
0b1935e
4015bb6
655410a
8fa39bf
39e4923
592f3af
fddbe2c
52e434a
cfdc548
956a563
a159cf1
3e0958b
eece107
39f9838
aba0eab
5382e88
7fc9be2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a
Geo_Point
structure in Enso stdlib? Some time ago we didn't.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we don't It has to be user defined. This is not new, just explicitly stating the current behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so let's put the description how the Geo_Point should look like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should remove Geo_Point support in this PR. It was a hack before maps support for tables. Please confirm it with Sylwia, but it should be dropped already
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How it is working for "old" datatypes like array of GeoPoints, or just json string?; As I see it assumes that
df
hasselect
andmap
method.I miss checking if type of df is a Table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now if the expression fails, we get the original data. Once we want to support multiple data types we can use
case
to transform depending on the input type.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, but that is an obscure feature I would say, and I feel it could change easily. So I would put here the case for Table explicitly and make
df.to_json.to_text
in other cases.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is refactored.