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

Fixing cocycle lifts with arbitrary integer coefficients #17

Merged
merged 1 commit into from
Aug 20, 2024

Conversation

vincent-grande
Copy link
Contributor

Hi, I have just looked at the portion of DREiMac's code responsible for fixing lifts from prime coefficients to integer coefficients, as discussed in [1]. DREiMac does this using a linear program and scipy.optimize.milp. However, no bounds are passed to the milp-solver, which defaults to all variables assumed to be non-negative.

I might be wrong here, but I don't see any reason to enforce the non-negativity. In particular, I can imagine there to be extraordinarily rare cases where a fix depends on negative values to be available.

I have implemented a similar fix to faulty lifted Z/3Z coefficients for homology generators (I.e. cycles) on alpha-complexes in my own project [2], and there the integer linear program required negativity of some of the variables to be feasible (On the TREFOIL knot from DREiMac's documentation).

I have added empty bounds to the uses of milp in complexprojectivecoords.py and toroidalcoords.py. Sorry if I misunderstood something here! (I have tested the proposed changes using the unit tests and the fix_cocycle notebook and both worked.)

[1] de Silva, V., Morozov, D. & Vejdemo-Johansson, M. Persistent Cohomology and Circular Coordinates. Discrete Comput Geom 45, 737–759 (2011).
[2] https://github.com/vincent-grande/topf

@LuisScoccola
Copy link
Collaborator

@vincent-grande good catch; indeed there is no reason the solution should be positive. Thanks a lot for realizing this, and fixing it!

As nitpicks: could you please revert the other two changes to toroidalcoords (this and this), and say "empty bounds" here (for consistency).

@vincent-grande
Copy link
Contributor Author

@LuisScoccola Thanks for the quick reply and sure! The other changes seemed to have been autoformatting, but I have reverted them.

@ctralie
Copy link
Member

ctralie commented Aug 20, 2024

Thank you so much @vincent-grande!
@LuisScoccola, I'll let you handle the pull request.
Also Luis, I wonder if this improves any examples that didn't work as well before. Do you have anything you thought should have worked better that this might improve?

@LuisScoccola
Copy link
Collaborator

@vincent-grande thanks again!

@ctralie it may be the case that fixing cocycles works faster now, since there are more solutions available. Also, it used to be the case that the algorithm would find no solution (or take a long time) when lifting cocycles with Z/2Z coefficients; I haven't had the chance to check this, but I feel like this might have been the issue.

@LuisScoccola LuisScoccola merged commit 39f2074 into scikit-tda:master Aug 20, 2024
14 checks passed
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