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

Open
wants to merge 101 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
Closes #123

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.
  • 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.
  • Document _array_functions.py.
  • Review documentation of objects.py
  • Review/write test docstrings.
  • Fix comoving == None prints as (Physical).
  • Look at recursion in cosmo_array.__new__ for lists of lists of cosmo_arrays, etc. (shouldn't recurse here because unyt doesn't, but __new__ needs some cleanup if possible anyway).
  • Implement bypass_validation?
  • Look in visualisation routines to address Projection/slicing returns unyt_array instead of cosmo_array #123 ?
  • Move the cosmo+unit wrapper for vis return images out of the backends and into the high-level render functions + update tests.
  • Review & expand narrative docs.
  • Review visualisation narrative docs.
  • Recruit some beta testers.
  • 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.
  • Pin dependencies (numpy >= 2.0.0 for sure, maybe >= 2.1.0; unyt >= 3.0.4).

@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 kyleaoman requested a review from JBorrow February 9, 2025 15:33
@robjmcgibbon
Copy link
Collaborator

Cool, thanks for this @kyleaoman! I'll have some time to look at it the week after next if Josh doesn't get to it before that

@kyleaoman
Copy link
Member Author

Thanks @robjmcgibbon!

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
2 participants