-
Notifications
You must be signed in to change notification settings - Fork 34
Update geomaps to allow dataframes as input. #1187
Conversation
* Can also consume an array of `Geo_Point` structures, or a dataframe that has the columns | ||
* `latitude`, `longitude` and `label`. |
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.
'df ->\n' + | ||
" columns = df.select ['label', 'latitude', 'longitude'] . columns\n" + | ||
" serialized = columns.map (c -> ['df_' + c.name, c.to_vector])\n" + | ||
' Json.from_pairs serialized . to_text' |
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
has select
and map
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.
Co-authored-by: Adam Obuchowicz <adam.obuchowicz@luna-lang.org>
* Can also consume an array of `Geo_Point` structures, or a dataframe that has the columns | ||
* `latitude`, `longitude` and `label`. |
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
src/rust/ide/view/graph-editor/src/builtin/visualization/java_script/geoMap.js
Outdated
Show resolved
Hide resolved
src/rust/ide/view/graph-editor/src/builtin/visualization/java_script/geoMap.js
Outdated
Show resolved
Hide resolved
src/rust/ide/view/graph-editor/src/builtin/visualization/java_script/geoMap.js
Outdated
Show resolved
Hide resolved
src/rust/ide/view/graph-editor/src/builtin/visualization/java_script/geoMap.js
Outdated
Show resolved
Hide resolved
…n review feedback.
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.
@@ -10,7 +10,6 @@ | |||
*/ | |||
const TOKEN = | |||
'pk.eyJ1IjoiZW5zby1vcmciLCJhIjoiY2tmNnh5MXh2MGlyOTJ5cWdubnFxbXo4ZSJ9.3KdAcCiiXJcSM18nwk09-Q' | |||
const GEO_POINT = 'Geo_Point' | |||
const GEO_MAP = 'Geo_Map' |
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.
As I remember, Geo_Map we should remove as well?
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.
Removed.
/** | ||
* Extract the visualisation data from a dataframe. | ||
*/ | ||
function extractFromDataFrame(parsedData, preparedDataPoints, accentColor) { |
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.
I think the name of the function could be better. The docs tell about extracting VISUALIZATION DATA. Lets add it to the name of the function as the name doesn't tell what we are extracting there.
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.
Done.
This is actually correct. These points are really far apart. And the default radius is to small to be seen at that distance. The example is broken. It has the coordinates for the viewport and the point to display switched. This has been fixed. |
c9b3e53
to
5382e88
Compare
Allow Tables to feed the Geo Maps visualization. Uses the columns longitude, latitude and label. Extracts the data through an Enso preprocessor. Original commit: enso-org/ide@1ec89ee
Pull Request Description
Allow Tables to feed the Geo Maps visualization. Uses the columns
longitude
,latitude
andlabel
.Extracts the data through an Enso preprocessor.
Important Notes
Also, some refactoring of the Geo Maps code.
Checklist
Please include the following checklist in your PR:
CHANGELOG.md
was updated with the changes introduced in this PR.All code has automatic tests where possible.All code has been manually tested in the "debug/interface" scene.