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

There should be a cosmo_quantity analog to unyt_quantity #102

Open
kyleaoman opened this issue Oct 18, 2021 · 9 comments
Open

There should be a cosmo_quantity analog to unyt_quantity #102

kyleaoman opened this issue Oct 18, 2021 · 9 comments

Comments

@kyleaoman
Copy link
Member

Many of the unyt_array functions can return a unyt_quantity in at least some circumstances. For a cosmo_array, this leads to the extra attributes being silently dropped, and then potential unexpected results or errors when trying to access those or use the resulting unyt_quantity in a calculation where the user expected a cosmo_array-like output.

@JBorrow
Copy link
Member

JBorrow commented Oct 19, 2021

Hmm, interesting. The unyt quantity is actually just an unyt array with one element by a different name. Good suggestion but I don't know how we'd get the functions inside unyt to return our Cosmo-Ised versions

@kyleaoman
Copy link
Member Author

I think it's not too difficult. I believe you have to wrap the functions one by one, but for the most part can be done with generic wrappers. Pretty sure unyt itself has to do pretty much the same thing with many of the numpy functions to make sure they return with units attached, maybe that could serve as a template. It's a bit of a long and tedious job, but should probably be done at some point.

@MatthieuSchaller
Copy link
Member

How would you handle quantities with the same units but that transform differently? for instance velocities and sound speed?

@kyleaoman
Copy link
Member Author

The transformation thing is I think a separate issue? This is just about having a scalar equivalent (cosmo_quantity) to the array type (cosmo_array).

@MatthieuSchaller
Copy link
Member

You are right indeed; it's a separate problem.

@jchelly
Copy link
Contributor

jchelly commented Apr 7, 2022

It would definitely be good to fix this. I've been trying to use cosmo_arrays to compute halo properties but many operations cause them to decay to unyt_arrays (e.g. functions returning scalars, as Kyle found) or lose their cosmo_factor and comoving attributes - e.g. np.sum, np.max, or arithmetic on arrays or array sections.

@jchelly
Copy link
Contributor

jchelly commented Apr 8, 2022

Another approach would be to try to deal with factors of a just using the facilities provided by unyt so we don't need a sub class. We could make a dimensionless unit 'a' and include it in the units returned by swiftsimio, although reading multiple snapshots might be a problem then. Maybe each snapshot would need a separate unit system/registry so that a can have a different value?

@JBorrow
Copy link
Member

JBorrow commented Apr 8, 2022

The reason that we have a separate cosmo_array quantity, rather than using the usual a as a dimensionless unit, is to keep track of it when we've taken it away. So you can convert something to physical, and then back to co-moving. This then nicely also avoids the issues of things like needing separate unit registries for different snapshots.

I agree though, that having a cosmo_quantity analogue would be good.

@jchelly
Copy link
Contributor

jchelly commented Apr 8, 2022

Ok, thanks, I hadn't thought of that. I knew there had to be a reason why you didn't do it that way.

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

No branches or pull requests

4 participants