rivetTDA / rivet

RIVET is a tool for Topological Data Analysis, in particular two-parameter persistent homology.
GNU General Public License v3.0
73 stars 24 forks source link

x-axis Label Issue for BRips in Multicritical Branch #95

Closed mlesnick closed 6 years ago

mlesnick commented 7 years ago

The current default label is "degree" but the code uses (number_of_points-degree) for the label. IIRC this was a hack to circumvent an issue with negative indexing. But this makes it very hard to compare the plots for different data sets. The label should be the actual degree (and should thus be larger on the left than on the right.)

It is perhaps worth noting that soon RIVET will have a feature that allows the user to specify the viewing region, rather than having this be given automatically (https://github.com/rivetTDA/rivet/issues/45). This is essential for comparing the outputs of two different RIVET computations. When working with this new feature, we will want to be able to specify the degree manually as an input parameter. This is a separate issue, but may be worth keeping in mind as we consider what this fix should be for this.

mlesnick commented 6 years ago

As I've continued to work on the code, especially on edits to Roy's contributions, I see that there are some (necessarily) awkward workarounds to deal with the fact that RIVET doesn't yet support descending bigrades. In specific, the Grade struct, which ought to store unsigned ints, is instead storing ints, because the methods which builds the parameter-free Rips bifiltation stores the negatives of the degrees that arise. In my modifications, this unsigned/int issue also spills over to the IndexMatrix class.

These are very low-level issues, but they are likely to cause confusion. They should be addressed as part of the changes proposed here.

mlesnick commented 6 years ago

Simon's work addresses this. We didn't do away with the use of signed ints, but it's good enough as is. This can be closed after the merge.

mlesnick commented 6 years ago

Simon's work addressing this is now merged.