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

_subdiagrams only returns diagrams of the last dim in homology_dimensions #85

Closed
nphilou opened this issue Nov 15, 2019 · 2 comments
Closed
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@nphilou
Copy link
Contributor

nphilou commented Nov 15, 2019

The _subdiagrams method can (and currently should) take a list of homology dimensions list but will only return the diagrams of the last element in the list.

https://github.com/giotto-ai/giotto-learn/blob/cda44bd79760106185f3a2849d6b36dbfb0a5aae/giotto/diagrams/_utils.py#L17-L23

Should we remove the loop or return an array of [subdiagrams_i, subdiagrams_j] with homology_dimensions = [i, j] as param ?

Also, this method can be very useful for users so maybe should we make it public ?

@ulupo
Copy link
Collaborator

ulupo commented Nov 20, 2019

It is also worth noting that this impacts performance, however slightly. Perhaps removing the loop and taking an integer instead of iterable as argument is the cleanest option. @gtauzin what do you think? I also agree that the method could be made public, but in that case I'm not sure how we would make it visible in the docs page.

@ulupo ulupo added the enhancement New feature or request label Jan 1, 2020
@ulupo ulupo added this to the 0.1.4 milestone Jan 1, 2020
@ulupo
Copy link
Collaborator

ulupo commented Aug 23, 2020

Fixed by #439.

@ulupo ulupo closed this as completed Aug 23, 2020
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

No branches or pull requests

3 participants