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

Small codebase updates #50

Merged
merged 23 commits into from
Dec 6, 2021
Merged

Conversation

MonkeyBreaker
Copy link
Collaborator

@MonkeyBreaker MonkeyBreaker commented Nov 14, 2021

This PR addresses multiples small issues here and there in the code base that where on my to-do list:

  • : Add templates to apparent pair related code to remove code logic duplication
  • : Add verification of coefficient parameter. Checks that the parameter is lower than the maximal value supported and if the value is a prime number
  • : Update naming of variables inside of binomial_coeff_table as requested by @ulupo
  • : Update barcodes returned from the bindings to directly be of Numpy type, because we want to match float 32-bit precision used instead of Python default one float 64-bit precision. The return value is therefore composed of Numpy arrays.
  • : Fixed an issue with recent version of GCC compiler with a header dependency that was removed from algorithm header.
  • : Remove factor parameter who was unused
  • : Remove num_edges return value who is unused and if needed, can be easily computed in Python.

julian added 9 commits November 10, 2021 22:14
The new names should also fix the assert in the operator method

Signed-off-by: julian <julian.burellaperez@heig-vd.ch>
Signed-off-by: julian <julian.burellaperez@heig-vd.ch>
…of 64

Signed-off-by: julian <julian.burellaperez@heig-vd.ch>
Signed-off-by: julian <julian.burellaperez@heig-vd.ch>
For removing some code duplication, I need to add a constructor to boundary_enumerator

Signed-off-by: julian <julian.burellaperez@heig-vd.ch>
Signed-off-by: julian <julian.burellaperez@heig-vd.ch>
Signed-off-by: julian <julian.burellaperez@heig-vd.ch>
Signed-off-by: julian <julian.burellaperez@heig-vd.ch>
@MonkeyBreaker
Copy link
Collaborator Author

@ulupo if you think of anything else that should be added to this PR.
Please let me know, I am not sure additional things can/should be added on this PR, it addresses small things I got in my to-do list.

Copy link
Collaborator

@ulupo ulupo left a comment

Choose a reason for hiding this comment

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

Great effort, thank you very much! A quick comment before a full review: I was under the impression that Ripser/ripser implemented its own is_prime function in C++. What is the reason for not incorporating it here?

@MonkeyBreaker
Copy link
Collaborator Author

I was under the impression that Ripser/ripser implemented its own is_prime function in C++. What is the reason for not incorporating it here

Well, the function is_prime was removed from the ripser source file.
In my opinion, a is_primefunction should be more in a utility file than inside of the ripser PH computation file.
The only reason I would think to have the C++ is_prime function back, is if this becomes a bottleneck computation.
@ulupo, what do you think ?

gph/src/ripser.h Outdated Show resolved Hide resolved
gph/src/ripser.h Outdated Show resolved Hide resolved
julian added 2 commits November 24, 2021 11:08
As suggested by @ulupo this method is not much faster

Signed-off-by: julian <julian.burellaperez@heig-vd.ch>
Signed-off-by: julian <julian.burellaperez@heig-vd.ch>
gph/python/ripser_interface.py Outdated Show resolved Hide resolved
gph/python/ripser_interface.py Outdated Show resolved Hide resolved
Julián and others added 2 commits November 24, 2021 22:11
Co-authored-by: Umberto Lupo <46537483+ulupo@users.noreply.github.com>
Store max coefficient value in a variable and use then the variable

Signed-off-by: julian <julian.burellaperez@heig-vd.ch>
Co-authored-by: Umberto Lupo <46537483+ulupo@users.noreply.github.com>
julian added 5 commits December 4, 2021 17:08
Signed-off-by: julian <julian.burellaperez@heig-vd.ch>
…nt dim

Signed-off-by: julian <julian.burellaperez@heig-vd.ch>
This could be easely achieved in Python

Signed-off-by: julian <julian.burellaperez@heig-vd.ch>
Signed-off-by: julian <julian.burellaperez@heig-vd.ch>
Signed-off-by: julian <julian.burellaperez@heig-vd.ch>
gph/src/ripser.h Outdated Show resolved Hide resolved
Julián and others added 2 commits December 4, 2021 17:33
Co-authored-by: Umberto Lupo <46537483+ulupo@users.noreply.github.com>
Signed-off-by: julian <julian.burellaperez@heig-vd.ch>
Copy link
Collaborator

@ulupo ulupo left a comment

Choose a reason for hiding this comment

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

Great work, thanks!

@ulupo ulupo merged commit ed70d4d into giotto-ai:main Dec 6, 2021
@MonkeyBreaker MonkeyBreaker deleted the small_codebase_updates branch December 8, 2021 21:24
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.

[CPP] Variable names in binomial_coeff_table Output floating point types
2 participants