mbaudin47 / othdrplot

High density plot
GNU Lesser General Public License v3.0
2 stars 3 forks source link

WIP: Contour with dim > 2 #17

Closed tupui closed 5 years ago

tupui commented 5 years ago

It closes #6 and also closes #15, closes #17.

tupui commented 5 years ago

@mbaudin47 I added the install instructions.

mbaudin47 commented 5 years ago

I tested the commit, which seems very promising.

It works on 2D data:

hdrplot-contour

But it fails on 3D sample.

For the ProcessHDR with 3 components, it seems to works at first:

processhdrplot-withoutarguments

But fails when tuning the options:

processhdrplot-drawdata

The core of the plot is done in drawContour of the HDRAlgo. I will add comments within the code in order to comment the details of the implementation, which has some minor flaws in my opinion.

Looking at the code, my feeling is that there is a little more work required to achieve the purpose. After experimenting a bit, I realized that we lack of a lower level class which would handle the messy work of visualizing an advanced Pairs with the distribution on the diagonal. With that building block, the only required work would be to add the contour on the extra diagonal.

In the following zip I put the work I prepared.

data-3D.zip

The gauss-mixture-3D.csv file contains a sample mixture of two well selected 3d gaussians for testing purposes. This is the output of the Pairs class in OT:

gaussian-mixture-3d

You will also find a drawPairsWithPDF function which fills the gap. On the same dataset, this produces:

pairs-with-pdf

So here is a suggested workplan:

What do you think of this ?

mbaudin47 commented 5 years ago

I have a working draft of MatrixPlot class: I commit in a dedicated branch ASAP.

tupui commented 5 years ago

I fixed the code and remove the dependency to itertools. The code should work as expected now. The only remaining issue I face is that the legend is duplicated and I don't find the root of this problem.

IMO, creating a class ot.PairWithDensity is not the solution as it will still require some work here to adapt all the subplots (only plot inliers or outliers, etc.). I agree that a more complete version of ot.Pairs should be present in OT. For the API, I would suggest to look at what's done in the seaborn library.

But I would say that this work is out of scope of the current module.

mbaudin47 commented 5 years ago

What do you think of PR #19 ?

tupui commented 5 years ago

What do you think of PR #19 ?

As I am saying in my previous comment. I don’t have the feeling that such a class is required. Just on the API point of view, I don’t see an intuitive way to modify the individual graphs then (we have to iterate over i, j and enumerate to have an index. So making sure we are modifying the correct subplot). This would better fit directly into OT.

mbaudin47 commented 5 years ago

I agree that this might be difficult to reuse. Please look at PR #20 for small modifications of your suggestions.