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

Complete cosmo_array numpy support and add cosmo_quantity #219

Draft
wants to merge 30 commits into
base: master
Choose a base branch
from

Conversation

kyleaoman
Copy link
Member

@kyleaoman kyleaoman commented Dec 12, 2024

Unyt has a unyt_quantity for scalars with units attached (a subclass of unyt_array). It makes sense to have an analogue cosmo_quantity subclass of cosmo_array, mainly so that various numpy functions don't silently cast to unyt_quantity. While I'm at it am also reviewing testing of numpy function return types and cleaning up helper functions as needed.

Closes #102

So far I have new tests implemented, and the new cosmo_quantity class implemented. As collateral to all of this I'm implementing wrapping all of the numpy functions supported by unyt. Those are now all wrapped. I'd previously wrapped all of the numpy ufuncs so most of the logic existed already, just repackaged existing helper functions and created a couple of new ones, and now cribbed off of the unyt wrappers to make sure I don't miss any edge cases (unless they missed any!).

After a bit of tidying up and a couple of squashed bugs in unyt the test suite now passes (with the not-yet-released unyt version with my patches applied, they say hopefully released by the end of the year). Will need to pin dependencies to unyt >=3.0.4, and probably numpy >=2.0, too.

Left to do:

  • Think about more test coverage.
  • Tidy up warnings in tests.
  • Consider refactoring all of the "ca_cfs" logic by using cosmo_factor(None, None) instead. That's a big job, though.
  • Look at a few places where to_comoving might be better as convert_to_comoving (in-place).
  • Look at behaviour of defaulting to comoving when mixed arguments are given, there's a kwarg now that can control this, should I use it elsewhere?
  • Tidy up definitions, probably move a bunch of helpers over from objects.py to _array_functions.py.
  • Check that everything is merged on unyt side and that they make a release. Will probably have to rename some stuff when they fix the fft->ftt typo.
  • Make a PR for BUG: numpy function take strips units with scalar index input yt-project/unyt#549 and get it merged, then implement a wrapper for np.take.
  • Docs.
  • Pin dependencies.
  • See if anything else needs doing :p

@kyleaoman kyleaoman added the enhancement New feature or request label Dec 12, 2024
@kyleaoman kyleaoman self-assigned this Dec 12, 2024
@kyleaoman kyleaoman changed the title Implement a cosmo_quantity analogous to unyt_quantity Complete cosmo_array numpy support and add cosmo_quantity Dec 15, 2024
@kyleaoman
Copy link
Member Author

@JBorrow just want to put this on your radar as it's basically implemented and working now (modulo notes in the post above). Gonna mostly leave it be over the xmas break, but hoping to wrap it up in January. Anyway just don't want to spring a colossal PR on you without warning - if you have any input or requests to make things go smoothly with it at this stage let me know?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

There should be a cosmo_quantity analog to unyt_quantity
1 participant