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

fix _eigs_jaxarray to be compatible with jit #52

Merged
merged 5 commits into from
Jun 17, 2024

Conversation

rochisha0
Copy link
Contributor

##Description
This PR is in an effort to enable jax.jit with qutip.core.metrics and entropy. It fixes _eigs_jaxarray to be compatible with jax.jit.

##Result
With this change trace_dist works with jax.jit

src/qutip_jax/linalg.py Outdated Show resolved Hide resolved
@rochisha0
Copy link
Contributor Author

@Ericgig I have made some changes, please have a look. As far as I tested this way we can work around jax.lax.cond.

Copy link
Member

@Ericgig Ericgig left a comment

Choose a reason for hiding this comment

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

This approach is looking good.

Have you confirmed that jax support grad for eigen problems?

src/qutip_jax/linalg.py Outdated Show resolved Hide resolved
@rochisha0
Copy link
Contributor Author

@Ericgig You are right I missed that jax only supports grad of eigenvalues not eigenvectors.

Copy link
Member

@Ericgig Ericgig left a comment

Choose a reason for hiding this comment

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

Now can eigs_jaxarray itself be jit if so let's move the jit from _eigs_jaxarray to there.
We use jnp.complex128 for data. Not jnp.complex64 causing the tests to fail.
How about astype(data.dtype)? This would be future proof if we decide to support both types.

@Ericgig
Copy link
Member

Ericgig commented Jun 13, 2024

Look good.
Does is work well when isherm=None, that is not included in our tests cases.

@rochisha0
Copy link
Contributor Author

@Ericgig I have added the suggested changes and the required tests are now passing.

@rochisha0
Copy link
Contributor Author

Yes, it works. @Ericgig

@Ericgig
Copy link
Member

Ericgig commented Jun 14, 2024

Could you add/modify a test for it.

Copy link
Member

@Ericgig Ericgig left a comment

Choose a reason for hiding this comment

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

Thank you.

@Ericgig Ericgig merged commit bd4f704 into qutip:master Jun 17, 2024
2 of 3 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.

2 participants