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

Conversation

Projects
None yet
2 participants
@philippjfr
Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor Author

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

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
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
You can’t perform that action at this time.