More consistent choice of a grid for diagram representations #719
Conversation
There was a problem hiding this comment.
The changes fully address the issue.
The definition of self.grid_ could be extracted to a common function for BettiCurve, Silhouette and Landscape. If you agree that it improves readability and reduces redundancy, then such changes could be included in this pr.
| self.nan_in_range = np.isnan(np.array(self.sample_range)) | ||
| self.new_resolution = self.resolution | ||
| if not self.keep_endpoints: | ||
| self.new_resolution += self.nan_in_range.sum() | ||
| self.sample_range = _automatic_sample_range(np.array(self.sample_range), X, y) | ||
| self.grid_ = np.linspace(self.sample_range[0], self.sample_range[1], self.resolution) | ||
| self.grid_ = np.linspace(self.sample_range[0], self.sample_range[1], self.new_resolution) | ||
| if not self.keep_endpoints: | ||
| self.grid_ = _trim_endpoints(self.grid_, self.nan_in_range) |
There was a problem hiding this comment.
This whole block is common to the three classes t,argeted by this PR and they could be extracted. I think it might improve readability.
This would require moving the first 3 lines from __init__ tofit for Landscape and Silhouette.
There was a problem hiding this comment.
Done. I temporarily had a small problem where calling fit twice (possibly forbidden, but happens naturally with __call__) would be different from just once, but I fixed that.
| """ | ||
| Constructor for the Landscape class. | ||
|
|
||
| Parameters: | ||
| num_landscapes (int): number of piecewise-linear functions to output (default 5). | ||
| resolution (int): number of sample for all piecewise-linear functions (default 100). | ||
| sample_range ([double, double]): minimum and maximum of all piecewise-linear function domains, of the form [x_min, x_max] (default [numpy.nan, numpy.nan]). It is the interval on which samples will be drawn evenly. If one of the values is numpy.nan, it can be computed from the persistence diagrams with the fit() method. | ||
| keep_endpoints (bool): when guessing `sample_range`, use the exact extremities (where the value is always 0). This is mostly useful for plotting, the default is to use a slightly smaller range. |
There was a problem hiding this comment.
Is guessing the right word? What about something like when `sample_range` is not specified (contains np.nan)? Or provided?
There was a problem hiding this comment.
Ok. I guess "guessing" doesn't sound professional 😉
I used "compute" because that's the word used in the description of sample_range on the previous line.
Actually, I missed |
Fix #715.