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

ENH: Point cloud function and plotting function #16

Merged
merged 15 commits into from
Mar 26, 2022
Merged

Conversation

ulupo
Copy link
Collaborator

@ulupo ulupo commented Mar 16, 2022

Fixes #14 and (partly) #13 and refactors notebooks accordingly.

Important: introduces GUDHI and plotly requirements at installation time.

Removes check_agreement_with_gudhi function which had been once used for debugging.

Plotting is achieved in Plotly and it creates a 2D persistence diagram containing the regular bars and the Steenrod bars simultaneously.

@ulupo ulupo requested review from ammedmar and gtauzin March 16, 2022 09:51
@ulupo
Copy link
Collaborator Author

ulupo commented Mar 16, 2022

I'm rerunning all notebooks from scratch (the cyclo-octane one takes very long) so we can verify that the barcodes remain the same as in the paper. Will post screenshots here.

@ulupo
Copy link
Collaborator Author

ulupo commented Mar 16, 2022

Klein bottle:

image
and
image

@ulupo
Copy link
Collaborator Author

ulupo commented Mar 16, 2022

Cyclo-octane:

image
and
image

@ammedmar please could you check that these and the ones for Klein look good, I've run out of time for now.

@ulupo ulupo added the enhancement New feature or request label Mar 16, 2022
@ulupo ulupo changed the title Point cloud function ENH: Point cloud function Mar 16, 2022
@ulupo ulupo changed the title ENH: Point cloud function ENH: Point cloud function and plotting function Mar 17, 2022
@ulupo
Copy link
Collaborator Author

ulupo commented Mar 17, 2022

@ammedmar I have pushed a plotting function, plot_diagrams.

You can test it on the Klein bottle notebook by adding the import line

from steenroder.plotting import plot_diagrams

and then e.g. by plotting the result of the last computation (R=1.3, absolute cohomology) via

plot_diagrams(barcode[:3], st_barcode[:3], k=1, kind="A")

@ulupo ulupo linked an issue Mar 17, 2022 that may be closed by this pull request
@ulupo ulupo removed a link to an issue Mar 17, 2022
Copy link
Collaborator

@gtauzin gtauzin left a comment

Choose a reason for hiding this comment

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

Lgtm! Just a small comment.

steenroder/plotting.py Outdated Show resolved Hide resolved
@ulupo
Copy link
Collaborator Author

ulupo commented Mar 21, 2022

Thanks @gtauzin! I realized I forgot to document rips_barcodes, so I just pushed a simple docstring.

@ulupo
Copy link
Collaborator Author

ulupo commented Mar 24, 2022

Testing on @ammedmar's machine shows that the plotting function does not work well in all systems. I think this is a MathJax-related issue because the function tries to produce LaTeX legend labels. The simplest way to fix this would be to agree on default legend label names that do not require LaTeX, and still allow LaTeX rendering as an option in case people want to try that and it works.

@ammedmar what do you think? What would be suitable names to replace $\mathcal{H}^i_A$, $\mathcal{H}^i_R$, $\mathrm{im}(Sq^k) \cap \mathcal{H}^i_A$, and $\mathrm{im}(Sq^k) \cap \mathcal{H}^i_R$, and that would not require LaTeX?

@ammedmar
Copy link
Collaborator

Yes, we can drop the LaTeX labels then. How about the following?
$\mathcal{H}^i_A$ --> H^i(abs)
$\mathcal{H}^i_R$ --> H^i(rel)
$\mathrm{im}(Sq^k) \cap \mathcal{H}^i_A$ --> im(Sq^k) in H^i(abs)
$\mathrm{im}(Sq^k) \cap \mathcal{H}^i_R$ --> im(Sq^k) in H^i(rel)

@ulupo
Copy link
Collaborator Author

ulupo commented Mar 24, 2022

@ammedmar I have now implemented your suggestion. Could you please check that the diagrams in the Klein bottle notebook (in this branch) display as expected for you? They display well at my end.

@ammedmar
Copy link
Collaborator

ammedmar commented Mar 25, 2022

I also tested it on the rpn an cp2 notebooks and it looks good. You were right about tex being the issue.
Two very minor comments:

  1. I realized that writing (abs) or (rel) in each line of the legend is maybe not ideal. Maybe one can just write absolute (relative) cohomology once as a title for the legend instead.
  2. The legend doesn't seem to scale to fit below the diagonal (see screenshot below).

I don't think either of these needs to be address in this PR though.

@ammedmar
Copy link
Collaborator

Screenshot from 2022-03-25 09-30-48

@ulupo
Copy link
Collaborator Author

ulupo commented Mar 25, 2022

Thanks @ammedmar!

Maybe one can just write absolute (relative) cohomology once as a title for the legend instead.

This is a good idea and would be one easy commit, I'll do it today.

The legend doesn't seem to scale to fit below the diagonal (see screenshot below).

This is harder and I'd indeed leave it to another PR -- I just note that one can make the whole figure bigger as a temporary fix.

@ulupo
Copy link
Collaborator Author

ulupo commented Mar 25, 2022

@ammedmar I've made the change. Let me know if happy to merge!

@ammedmar
Copy link
Collaborator

Yes, it looks good!

@ulupo ulupo merged commit 8705198 into main Mar 26, 2022
@ulupo ulupo deleted the point_cloud_function branch March 26, 2022 09:22
@ulupo ulupo mentioned this pull request Mar 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Point cloud
3 participants