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

Follow conjugation convention in tree_dot #105

Merged
merged 2 commits into from
Aug 18, 2024

Conversation

Randl
Copy link
Contributor

@Randl Randl commented Aug 17, 2024

It may break something downstream unfortunately, so updating dependencies should be done carefully

@patrick-kidger
Copy link
Owner

Modulo failing tests -- not sure what's going on with those (?) -- this LGTM.
Once the tests are fixed we can check the Optimistix and Diffrax tests on this PR before merging it, and see if anything looks awry.

@Randl
Copy link
Contributor Author

Randl commented Aug 18, 2024

Should we bump the version since this is a potentially breaking change?

@patrick-kidger
Copy link
Owner

Yes, I think we should.

I can see that tests now pass -- are you happy with this PR / should I merge it?

@Randl
Copy link
Contributor Author

Randl commented Aug 18, 2024

I think I am.
Though one possibility to consider is to not do conjugation in tree_dot at all but rater leave it to caller, similarly to jnp.dot. This may clutter downstream code but will be consistent with the Jax/numpy semantics.

@patrick-kidger patrick-kidger merged commit 5e79294 into patrick-kidger:main Aug 18, 2024
2 checks passed
@patrick-kidger
Copy link
Owner

Merged, then!
And that's an interesting idea. In particular given the difficulties we faced in patrick-kidger/optimistix#61, I can see that that might simplify things.

The decision is yours -- I'd be happy to make that change if you think that is what is best.

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.

2 participants