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

Operation fixes #1093

Merged
merged 7 commits into from Feb 1, 2017
Merged

Operation fixes #1093

merged 7 commits into from Feb 1, 2017

Conversation

@philippjfr
Copy link
Member

@philippjfr philippjfr commented Jan 31, 2017

A number of small fixes for existing operations:

  1. Disable the group being inherited from composite types as would sometimes occur when applying operations to an NdOverlay resulting in Element being labeled "NdOverlay".

  2. Hide warning in datashade operation when shading a categorical aggregate.

  3. Removed the adjoin parameter to histogram operation, it continually annoys me because the hist method has to handle the adjoining itself anyway and having the operation return an adjoined object is a) weird and b) actually breaks if you're trying to apply the operation to a HoloMap or other kind of composite object.

@philippjfr philippjfr force-pushed the operation_fixes branch 2 times, most recently from 2afe29a to 51a56b4 Jan 31, 2017
@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Jan 31, 2017

Ready to review.

@philippjfr philippjfr requested a review from jlstevens Jan 31, 2017
@philippjfr philippjfr force-pushed the operation_fixes branch from 51a56b4 to 144b7e2 Jan 31, 2017
@jlstevens
Copy link
Contributor

@jlstevens jlstevens commented Jan 31, 2017

Looks good though I am worried about backwards compatibility implications for point 3. I agree with your rationale for removing the adjoin option but is anyone using it?

How about checking if adjoin is in the kwargs and issuing a useful exception message with some advice? Maybe something like 'Adjoin option deprecated: use the << operator to explicitly adjoin output histogram'? That way you can remove the functionality without having the option vanish without a trace...

@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Jan 31, 2017

How about checking if adjoin is in the kwargs and issuing a useful exception message with some advice? Maybe something like 'Adjoin option deprecated: use the << operator to explicitly adjoin output histogram'?

I suspect people were sufficiently discouraged from using. I'm happy to add the exception but in fixing it I realized the operation itself was fairly useless in all kinds of ways, auto-ranging was disabled for instance so without supplying an explicit bin_range or None you'd get a histogram ranging from 0 to 1.

@jlstevens
Copy link
Contributor

@jlstevens jlstevens commented Jan 31, 2017

Ok. In that case, I am happy to merge...although the pr build does seem to have failed right now...

@philippjfr
Copy link
Member Author

@philippjfr philippjfr commented Jan 31, 2017

One of the many transients we're struggling with again, about 1/4 builds fail.

@jlstevens jlstevens merged commit 82a8d13 into master Feb 1, 2017
4 checks passed
4 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 77.906%
Details
@philippjfr
s3-reference-data-cache Test data is cached.
Details
@philippjfr philippjfr deleted the operation_fixes branch Feb 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants