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

New diagonal_operation-Parameter for the gridmatrix operation #1027

Merged
merged 1 commit into from Feb 24, 2017

Conversation

Projects
None yet
2 participants
@mrksr
Contributor

mrksr commented Dec 21, 2016

Instead of being restricted to a plot type along the diagonal of a gridmatrix-operation, this pull request also allows the specification of an operation. The main reasoning for this is that it is now possible to change parameters like the number of bins in a diagonal histogram:

data = pd.DataFrame(np.random.normal(size=(250, 3)), columns=['a', 'b', 'c'])
hvset = hv.Dataset(data.reset_index(), kdims=['index'])

hv.operation.gridmatrix(
    hvset,
    diagonal_operation=hv.operation.histogram.instance(num_bins=30, adjoin=False)
)

I think in its current state, this PR is not ready however, since there are a few open questions:

  • Should diagonal_type remain?
  • Should there be a chart_operation parameter?
  • How to formulate the docstrings to be easy to understand?
  • Is the normalization relevant for the "operation-histograms"?

When the semantics are decided, I can also add a few tests. I will probably need a few hints however on how to test this.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jan 7, 2017

Sorry I've been so slow in reviewing this, I'll get to it over the weekend.

bin_range = ranges.get(d1.name, element.range(d1))
el = p.diagonal_operation(element,
dimension=d1.name,
bin_range=bin_range)

This comment has been minimized.

@philippjfr

philippjfr Jan 16, 2017

Member

I'm afraid this won't behave very well unless axiswise normalization is enabled here.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jan 16, 2017

Sorry for the long delay, this looks fine, except one small comment. If you don't think you'll have the time I'm happy to merge anyway and apply the change myself.

@mrksr

This comment has been minimized.

Contributor

mrksr commented Jan 19, 2017

Your comment should be fixed now.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jan 19, 2017

Perfect thanks! I'm actually going to clean up the histogram operation a bit to remove the adjoin nonsense so this can be cleaned up further. For the time being this looks good and I'll merge as soon as tests are passing.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jan 19, 2017

Unless you feel like writing some simple tests that is?

@mrksr

This comment has been minimized.

Contributor

mrksr commented Jan 19, 2017

I'm not sure on how to test this. As I cannot find tests for the other operations, can you point to something similar?

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jan 19, 2017

You're right, it appears operations are not currently tested at all! I'd create a new file called testoperation.py and then subclass the ComparisonTestCase class. The assertEqual on that class knows how to compare HoloViews objects so a few tests that manually construct GridMatrix and compare it to the gridmatrix output should be fine. I'd be happy to add some basic operation tests myself but will probably not get to it until fairly late today.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Feb 13, 2017

Could you rebase the PR, I'll get this merged and add unit tests separately.

@mrksr

This comment has been minimized.

Contributor

mrksr commented Feb 13, 2017

Sorry I did not find the time to look into the tests yet. I will do the rebase tomorrow!

@mrksr

This comment has been minimized.

Contributor

mrksr commented Feb 14, 2017

@philippjfr I rebased with 144b7e2 in mind.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Feb 24, 2017

Sorry this has stalled so long, I think tests are still failing because framewise normalization is not enabled. I'll fix the tests in a separate PR.

@philippjfr philippjfr merged commit 3afcf01 into ioam:master Feb 24, 2017

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
s3-reference-data-cache Test data not cached, see details to rebuild.
Details

@mrksr mrksr deleted the mrksr:gridmatrix-diagonal-operation branch Feb 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment