Skip to content
This repository has been archived by the owner on Dec 28, 2021. It is now read-only.

Update geomaps to allow dataframes as input. #1187

Merged
merged 21 commits into from
Feb 16, 2021
Merged

Conversation

MichaelMauderer
Copy link
Contributor

@MichaelMauderer MichaelMauderer commented Feb 9, 2021

Pull Request Description

Allow Tables to feed the Geo Maps visualization. Uses the columns longitude, latitude and label.
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:

  • The CHANGELOG.md was updated with the changes introduced in this PR.
  • The documentation has been updated if necessary.
  • All code conforms to the Rust style guide.
  • All code has automatic tests where possible.
  • All code has been profiled where possible.
  • All code has been manually tested in the IDE.
  • All code has been manually tested in the "debug/interface" scene.
  • All code has been manually tested by the PR owner against our test scenarios.
  • All code has been manually tested by at least one reviewer against our test scenarios.

@MichaelMauderer MichaelMauderer changed the title feat: Update geomaps to allow dataframes as input. Update geomaps to allow dataframes as input. Feb 9, 2021
@MichaelMauderer MichaelMauderer self-assigned this Feb 9, 2021
@MichaelMauderer MichaelMauderer added Category: Visualizations Visualizations embedded in Enso IDE Difficulty: Core Contributor Should only be attempted by a core contributor Priority: High Should be scheduled as soon as possible Type: Enhancement An enhancement to the current state of Enso IDE labels Feb 9, 2021
@MichaelMauderer MichaelMauderer marked this pull request as ready for review February 10, 2021 13:52
Comment on lines 113 to 114
* Can also consume an array of `Geo_Point` structures, or a dataframe that has the columns
* `latitude`, `longitude` and `label`.
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

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.

Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is removed.

Comment on lines 166 to 169
'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'
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is refactored.

CHANGELOG.md Outdated Show resolved Hide resolved
MichaelMauderer and others added 2 commits February 11, 2021 15:10
Co-authored-by: Adam Obuchowicz <adam.obuchowicz@luna-lang.org>
CHANGELOG.md Outdated Show resolved Hide resolved
Comment on lines 113 to 114
* Can also consume an array of `Geo_Point` structures, or a dataframe that has the columns
* `latitude`, `longitude` and `label`.
Copy link
Member

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

@farmaazon
Copy link
Collaborator

On my machine the example with json string does not work.
image

Copy link
Collaborator

@farmaazon farmaazon left a comment

Choose a reason for hiding this comment

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

And when I create such table, I don't see any points on the map.

image

@@ -10,7 +10,6 @@
*/
const TOKEN =
'pk.eyJ1IjoiZW5zby1vcmciLCJhIjoiY2tmNnh5MXh2MGlyOTJ5cWdubnFxbXo4ZSJ9.3KdAcCiiXJcSM18nwk09-Q'
const GEO_POINT = 'Geo_Point'
const GEO_MAP = 'Geo_Map'
Copy link
Collaborator

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?

Copy link
Contributor Author

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) {
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@MichaelMauderer
Copy link
Contributor Author

And when I create such table, I don't see any points on the map.

image

This is actually correct. These points are really far apart. And the default radius is to small to be seen at that distance.

On my machine the example with json string does not work.
image

The example is broken. It has the coordinates for the viewport and the point to display switched. This has been fixed.

@farmaazon farmaazon merged commit 1ec89ee into develop Feb 16, 2021
@farmaazon farmaazon deleted the wip/mm/ide-1055 branch February 16, 2021 10:48
mwu-tow pushed a commit to enso-org/enso that referenced this pull request Oct 30, 2021
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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Category: Visualizations Visualizations embedded in Enso IDE Difficulty: Core Contributor Should only be attempted by a core contributor Priority: High Should be scheduled as soon as possible Type: Enhancement An enhancement to the current state of Enso IDE
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants