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

Remove sparse option in Graph geometry #329

Merged
merged 5 commits into from
Mar 8, 2023

Conversation

michalk8
Copy link
Collaborator

@michalk8 michalk8 commented Mar 6, 2023

closes #328

@michalk8 michalk8 added bug Something isn't working deprecation labels Mar 6, 2023
@michalk8 michalk8 self-assigned this Mar 6, 2023
@codecov-commenter
Copy link

codecov-commenter commented Mar 6, 2023

Codecov Report

Merging #329 (259d10e) into main (ce0301e) will decrease coverage by 0.07%.
The diff coverage is 97.14%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #329      +/-   ##
==========================================
- Coverage   88.65%   88.59%   -0.07%     
==========================================
  Files          52       51       -1     
  Lines        5668     5479     -189     
  Branches      864      824      -40     
==========================================
- Hits         5025     4854     -171     
+ Misses        518      509       -9     
+ Partials      125      116       -9     
Impacted Files Coverage Δ
src/ott/geometry/graph.py 94.79% <97.14%> (+2.91%) ⬆️
src/ott/math/utils.py 85.71% <0.00%> (-8.17%) ⬇️

@michalk8 michalk8 requested a review from marcocuturi March 6, 2023 17:47
Copy link
Contributor

@marcocuturi marcocuturi left a comment

Choose a reason for hiding this comment

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

thanks Michal, is there a reason (apart from the fact that it's not that clean) that we should remove this? any bugs this is causing?

@michalk8
Copy link
Collaborator Author

michalk8 commented Mar 7, 2023

thanks Michal, is there a reason (apart from the fact that it's not that clean) that we should remove this? any bugs this is causing?

Yes, there are some bugs with the current sparse implementation sometimes not working (because of the factor cache).

@marcocuturi
Copy link
Contributor

Ok LGTM then!

@marcocuturi marcocuturi merged commit 1c63892 into ott-jax:main Mar 8, 2023
michalk8 added a commit that referenced this pull request Jun 27, 2024
* Remove sparse option from `Graph`

* Fix typos

* Update graph tests

* Udpate docs, remove suitesparse from CI

* Update references, fix docs linter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove sparse option in the Graph geometry
3 participants