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

New diagonal_operation-Parameter for the gridmatrix operation #1027

Merged
merged 1 commit into from Feb 24, 2017
Merged

New diagonal_operation-Parameter for the gridmatrix operation #1027

merged 1 commit into from Feb 24, 2017

Conversation

mrksr
Copy link
Contributor

@mrksr 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
Copy link
Member

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)
Copy link
Member

Choose a reason for hiding this comment

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

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

@philippjfr
Copy link
Member

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
Copy link
Contributor Author

mrksr commented Jan 19, 2017

Your comment should be fixed now.

@philippjfr
Copy link
Member

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
Copy link
Member

Unless you feel like writing some simple tests that is?

@mrksr
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

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

@mrksr
Copy link
Contributor Author

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
Copy link
Contributor Author

mrksr commented Feb 14, 2017

@philippjfr I rebased with 144b7e2 in mind.

@philippjfr
Copy link
Member

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 holoviz:master Feb 24, 2017
@mrksr mrksr deleted the gridmatrix-diagonal-operation branch February 24, 2017 21:41
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.

None yet

2 participants