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

another stab at getting barcodes into the mix #24

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

DeliciousHair
Copy link

@DeliciousHair DeliciousHair commented Sep 4, 2019

I noticed that the current PR #22 for a barcode visualisation is failing, so I thought I'd try to fix up whatever the problem is, and in the process decided make the thing a little more to my tastes. Main change is that the H0 and H1 barcodes are combined in a single plot with finer lines than previous (since this is the most common scenario to examine) and individual plots for each dimension are produced if dim != 2.

As well, the optional export_png keyword argument in the plot_bacode() method will cause a PNG version of the barcode to be returned as an io.BytesIO() object.

Example use is included in the docstring for the Barcode class:

n = 300
t = np.linspace(0, 2 * np.pi, n)
noise = np.random.normal(0, 0.1, size=n)

data = np.vstack([((3+d) * np.cos(t[i]+d), (3+d) * np.sin(t[i]+d)) for i, d in enumerate(noise)])
diagrams = ripser(data)

bc = Barcode(diagrams['dgms'])
bc.plot_barcode()

@codecov-io
Copy link

codecov-io commented Sep 4, 2019

Codecov Report

Merging #24 (4702251) into master (c35fa04) will decrease coverage by 3.49%.
The diff coverage is 15.27%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #24      +/-   ##
==========================================
- Coverage   75.90%   72.41%   -3.50%     
==========================================
  Files          20       20              
  Lines        1457     1526      +69     
  Branches      298      317      +19     
==========================================
- Hits         1106     1105       -1     
- Misses        293      360      +67     
- Partials       58       61       +3     
Impacted Files Coverage Δ
persim/visuals.py 44.08% <15.27%> (-25.15%) ⬇️
persim/bottleneck.py 95.77% <0.00%> (-2.82%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c35fa04...4702251. Read the comment docs.

@DeliciousHair
Copy link
Author

I see, failure due to lack of tests written. Any ideas how I can address this?

@sauln
Copy link
Member

sauln commented Sep 5, 2019

Thanks for this PR!!

I wouldn't worry too much about the codecov results. Testing plotting code can be pretty nebulous.

Could you post come screenshots of the output this produces into a comment?

@DeliciousHair
Copy link
Author

Sure, easy! Here is the output from the sample code provided:

sample_barcode

@sauln
Copy link
Member

sauln commented Sep 5, 2019

Awesome! Thanks again for picking this up!

Does it look the same if you plot above $H_1$?

Would it be easy to make the x-axes the same for all of the sets? I could see it being helpful for comparisons, but also annoying if each have different scales. What do you think?

In the $H_1$ barcode, the bars are so thin it was hard to see. How difficult is it to make the width of each bar determined by the number of bars shown?

@DeliciousHair
Copy link
Author

Awesome! Thanks again for picking this up!

No problem, I was using the feature after all

Does it look the same if you plot above $H_1$?

No, if dim != 2 then individual plots are produced for each dimension. I can extend this up to $H_2$ trivially, but beyond that things get a bit large, so I figured that combining $H_0$ and $H_1$ was a good idea, everything else left on and individual basis.

Would it be easy to make the x-axes the same for all of the sets? I could see it being helpful for comparisons, but also annoying if each have different scales. What do you think?

Very easy, but the question arises about the number of bars for small $\epsilon$ values. If you want to share the axis and still have it readable, I should also make it possible for the user to specify the number of bars they want to to see so that the low-$\epsilon$ junk is truncated off, optionally of course.

In the $H_1$ barcode, the bars are so thin it was hard to see. How difficult is it to make the width of each bar determined by the number of bars shown?

The linewidth is already controllable via the linewidth keyword argument, but making that a function of n_bars would be a bit tricky. See above comment for controlling what is displayed; if that is added then the user could specify the linewidth as well in order to get something visually appealing.

Would you like me to do that?

Copy link
Member

@sauln sauln left a comment

Choose a reason for hiding this comment

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

Sorry for the long delay getting back to you about this. This all looks really good!

I added some comments. Mostly small changes to the api.

Once those are done, could you try generating the docs site to make sure it looks how you want? You should be able to run sphinx in the docs directory and then open up the generated html in your local browser.

I think to get the additions to render, you'll have to add a line or two in the docs rst files. Let me know if you need any help with that.

persim/visuals.py Outdated Show resolved Hide resolved
persim/visuals.py Outdated Show resolved Hide resolved
persim/visuals.py Outdated Show resolved Hide resolved
persim/visuals.py Outdated Show resolved Hide resolved
persim/visuals.py Outdated Show resolved Hide resolved
persim/visuals.py Outdated Show resolved Hide resolved
persim/visuals.py Outdated Show resolved Hide resolved
persim/visuals.py Outdated Show resolved Hide resolved
persim/visuals.py Show resolved Hide resolved
@DeliciousHair
Copy link
Author

DeliciousHair commented Aug 6, 2021

Sorry for abandoning this, was far too busy at the time to deal with it, then forgot all about it. Saw it today and thought I should fix it up. Better late than never!

* Code layout changed to play nice with sphinx docs template used
* removed useless `plot_a_bar` function
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.

3 participants