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

[ENH] Warn when data subset is unrelated to data #3507

Merged
merged 1 commit into from
Jan 4, 2019

Conversation

janezd
Copy link
Contributor

@janezd janezd commented Dec 28, 2018

Issue

Fixes #3292.

Also switches to np.fromiter when constructing a subset mask to avoid creating a Python list.

Includes
  • Code changes
  • Tests

@codecov
Copy link

codecov bot commented Dec 28, 2018

Codecov Report

Merging #3507 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3507      +/-   ##
==========================================
+ Coverage   83.51%   83.52%   +<.01%     
==========================================
  Files         367      367              
  Lines       65599    65635      +36     
==========================================
+ Hits        54787    54819      +32     
- Misses      10812    10816       +4

vstack(collect("X")),
merge1d(collect("Y")),
vstack(collect("metas")),
merge1d(collect("W"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we even support weights anywhere? 🤔

@ajdapretnar
Copy link
Contributor

This PR seems to contain several disjointed commits. Is this supposed to be so?

@janezd
Copy link
Contributor Author

janezd commented Jan 4, 2019

PR is based on #3500. Check just the last commit, af0f2d7 (DataProjectionWidget: Warn about invalid subset data).

@janezd janezd force-pushed the projection-invalid-subset branch from af0f2d7 to 97d5bbd Compare January 4, 2019 12:35
@janezd
Copy link
Contributor Author

janezd commented Jan 4, 2019

Rebased to master that already includes #3500.

@ajdapretnar
Copy link
Contributor

This works well!
It would probably make sense to implement the same warning in Line Plot and perhaps reword the warning for Mosaic Display to be the same as this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants