Skip to content

Conversation

TheSkyentist
Copy link
Contributor

A draft for Python 3.13 compatibility. Currently solving none of the bugs, but simply updating the CI matrix to test all versions of supported Python and updating the pyproject.toml to have the 3.13 tag while disallowing 3.14.

@TheSkyentist
Copy link
Contributor Author

Moving the discussion from #304 here. The current issue is with @AbstractQuantity.from_._f.register:

@AbstractQuantity.from_._f.register # noqa: SLF001

@AbstractQuantity.from_._f.register # type: ignore[no-redef] # noqa: SLF001

@AbstractQuantity.from_._f.register # type: ignore[no-redef] # noqa: SLF001

@AbstractQuantity.from_._f.register # noqa: SLF001

@AbstractQuantity.from_._f.register # type: ignore[no-redef] # noqa: SLF001

@TheSkyentist
Copy link
Contributor Author

The error, presented in full in #303, is an AttributeError: 'function' object has no attribute 'register'. Replacing @AbstractQuantity.from_.f.register with @dispatch causes interoperability problems and replacing with @AbstractQuantity.from._f.dispatch results in the same error.

@TheSkyentist
Copy link
Contributor Author

Perhaps this is an issue with plum, but testing the package itself with 3.12 and 3.13 yields the same results, but it may be due to something more complex.

Copy link

codecov bot commented Nov 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.26%. Comparing base (00d5841) to head (9da4dc5).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #307   +/-   ##
=======================================
  Coverage   96.26%   96.26%           
=======================================
  Files          43       43           
  Lines        1579     1579           
  Branches       72       72           
=======================================
  Hits         1520     1520           
  Misses         47       47           
  Partials       12       12           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nstarman nstarman added this to the v1.0.0 milestone Nov 26, 2024
@nstarman
Copy link
Contributor

I did some quick testing and it looks like beartype/plum#200 will fix this, allowing for from._f.register -> from_.dispatch

@TheSkyentist
Copy link
Contributor Author

I can confirm that this appears to solve most issues! But some critical tests still fail:

There seem to be issues related to unit conversions where the resultant JAX array has the weak_type flag set to true but the example doesn't expect this.
In addition, numpy operations appear to strip the unxt unit from the array.

@nstarman
Copy link
Contributor

This PR can now bump plum >= 2.5.4 2 and change to from_.dispatch. Then we can see what test failures remain.

@nstarman
Copy link
Contributor

Great! It looks like everything passes.

There seem to be issues related to unit conversions where the resultant JAX array has the weak_type flag set to true but the example doesn't expect this.
In addition, numpy operations appear to strip the unxt unit from the array.

Where were these issues showing up? Given the passing tests this sounds like issues for a followup PR.

@nstarman nstarman marked this pull request as ready for review November 26, 2024 16:24
nstarman
nstarman previously approved these changes Nov 26, 2024
@TheSkyentist
Copy link
Contributor Author

I'm glad it's passing all of the CIs! These issues are showing up when I run pytest locally. After doing a fresh install on this branch here is the issue I still have:

import unxt as u
x = u.Quantity(1000.0, "m")

returns a

Quantity['length'](Array(1000, dtype=int32, weak_type=True), unit='m')

note the weak_type flag being set.

This causes pytest failures as the sybil checks want the internal JAX array to be strongly typed. I'm unclear if this is an actual issue, but I thought it would be worth mentioning. If it's outside the scope of this PR then we can merge and get Python 3.13 support rolling. If it's not a problem at all, then great!

@TheSkyentist TheSkyentist dismissed nstarman’s stale review November 26, 2024 18:17

The merge-base changed after approval.

@TheSkyentist
Copy link
Contributor Author

I'm sorry for the spam! I tried to learn how to sign a commit and I forced pushed quite a few times.

@nstarman
Copy link
Contributor

I'm sorry for the spam! I tried to learn how to sign a commit and I forced pushed quite a few times.

No worries!

If it's outside the scope of this PR then we can merge and get Python 3.13 support rolling. If it's not a problem at all, then great!

It is indeed. Worth investigating. The weak dtype stuff is nice in that it indicates that copies are not being made.

@nstarman nstarman merged commit 7dabe2d into GalacticDynamics:main Nov 26, 2024
26 checks passed
@nstarman
Copy link
Contributor

Thanks @TheSkyentist for the nice PRs!

@TheSkyentist
Copy link
Contributor Author

Happy to help!

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