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 non-cython method and get rid of the spa class #50

Merged
merged 24 commits into from
Oct 16, 2024

Conversation

j-i-l
Copy link
Collaborator

@j-i-l j-i-l commented Oct 12, 2024

Closes #45

Closes #44

@j-i-l j-i-l added this to the JOSS paper submission milestone Oct 12, 2024
@j-i-l j-i-l self-assigned this Oct 12, 2024
@j-i-l j-i-l linked an issue Oct 12, 2024 that may be closed by this pull request
@j-i-l
Copy link
Collaborator Author

j-i-l commented Oct 12, 2024

All non-cython implementations are now exported to _cython_subst.py which we can easily drop if we want.

flowstab.SPA is now only still used in the _cython_sparse_stoch.stoch_mat_add/sub functions and called in sparse_stoch_mat.__add__ and sparse_stoch_mat.__sub__.

@alexbovet should we rewrite _cython_sparse_stoch.stoch_mat_add/sub to get rid of flowstab.SPA completely?

@j-i-l j-i-l requested a review from alexbovet October 12, 2024 21:14
@j-i-l
Copy link
Collaborator Author

j-i-l commented Oct 12, 2024

might need some adaptations once #49 is merged

@alexbovet
Copy link
Owner

Let's keep flowstab.SPA for the moment until we do some proper testing of it.

might need some adaptations once #49 is merged

What do you mean? can we merge this?

@j-i-l
Copy link
Collaborator Author

j-i-l commented Oct 15, 2024

Let's keep flowstab.SPA for the moment until we do some proper testing of it.

might need some adaptations once #49 is merged

What do you mean? can we merge this?

The changes from #49 partially affect the same files, so we just need to update this branch and sort out the potential conflicts, then this is ok to merge.
I'll update and check it.

@j-i-l
Copy link
Collaborator Author

j-i-l commented Oct 15, 2024

@alexbovet now is fine.

I'll then bring the branch for #12 up to date and the we can merge in all the testing

@alexbovet alexbovet merged commit 442e51d into main Oct 16, 2024
5 of 6 checks passed
@alexbovet alexbovet deleted the 45-get-rid-of-the-spa-class branch October 16, 2024 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants