Skip to content
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.

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

Fix/mapper colors #277

Merged
merged 5 commits into from
Feb 12, 2020
Merged

Fix/mapper colors #277

merged 5 commits into from
Feb 12, 2020

Conversation

lewtun
Copy link
Collaborator

@lewtun lewtun commented Feb 11, 2020

Reference Issues/PRs

Fixes #266

What does this implement/fix? Explain your changes.

  • Normalises node_colors values to lie in range [0,1]
  • Reuses plot_point_cloud function from examples/ for Mapper quickstart notebook

Any other comments?

@lewtun lewtun added the mapper label Feb 11, 2020
@lewtun lewtun requested a review from wreise February 11, 2020 15:16
@lewtun
Copy link
Collaborator Author

lewtun commented Feb 11, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Collaborator

@wreise wreise left a comment

Choose a reason for hiding this comment

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

Please clarify the comment on indentation. Reducing indentation produces the desired behavior on the us-election use case.

Comment on lines 140 to 143
# normalise node colours in range [0,1] for colorscale
node_colors = (node_colors - np.min(node_colors)) / \
(np.max(node_colors) - np.min(node_colors))

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe that this block is too indented and is executed only when not is_node_colors_ndarray, while I believe it should always be.

Same question for the above: should we get_node_summary only in the else case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch - thanks! I've fixed the indentation in cc2d646

Regarding get_node_summary it is correct to only call it in the else case as we distinguish between scenarios where the user passes an ndarray vs [callable, transformer, etc]. In the latter case we define color_data which is then used by get_node_summary to calculate node_colors

Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks for clarifying!

@lewtun
Copy link
Collaborator Author

lewtun commented Feb 12, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@lewtun
Copy link
Collaborator Author

lewtun commented Feb 12, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@wreise
Copy link
Collaborator

wreise commented Feb 12, 2020

Pipelines fail due to [IPKernelApp] WARNING | Parent appears to have exited, shutting down "papermill". Have we seen this before?
Some relevant issues:
1.
2.

@ulupo
Copy link
Collaborator

ulupo commented Feb 12, 2020

Hmm, no idea @wreise, this has never come up before. Maybe a freak accident and we can try rerunning?

@wreise
Copy link
Collaborator

wreise commented Feb 12, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@wreise wreise merged commit 37de219 into giotto-ai:master Feb 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Colors in plot_*_mapper_graph not normalized
3 participants