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

Projection/slicing returns unyt_array instead of cosmo_array #123

Open
bwvdnbro opened this issue Apr 19, 2022 · 4 comments
Open

Projection/slicing returns unyt_array instead of cosmo_array #123

bwvdnbro opened this issue Apr 19, 2022 · 4 comments
Assignees

Comments

@bwvdnbro
Copy link
Member

The projection and slicing functions return an unyt_array instead of a cosmo_array. Because of this, the conversion from co-moving to physical coordinates needs to be done manually. This is also a bit misleading, since this gives the impression that the resulting maps are in physical coordinates, while they are not if the coordinates in the snapshot are co-comoving.

Is there any particular reason for this? Can we just replace unyt_array with cosmo_array to fix this or does that cause other issues?

@JBorrow
Copy link
Member

JBorrow commented Apr 19, 2022

You can do that, but you'll have to be careful to copy over the metadata. I was also not so sure what to do in the case of comoving/physical conflicts. The length scales are usually all co-moving, but the properties may be physical. You'd also likely want to make a check that hsml and the length scales are all in the same frame.

@bwvdnbro
Copy link
Member Author

I discussed this with @MatthieuSchaller and we think the best option would be to just throw an error if someone tries to use physical and co-moving quantities together.

But I guess what you are saying is that a quantity like "mass" can be physical and still be meaningfully used together with co-moving length scales? So simply checking that all quantities involved have the same value for the comoving flag (and the same cosmo_factor) is not sufficient?

@MatthieuSchaller
Copy link
Member

How about everything should be:

  • "physical or transform as a^0" and then return the data in physical
  • "comoving or transform as a^0" and then return the data in comoving

Error if not in either of these.

@JBorrow
Copy link
Member

JBorrow commented Apr 21, 2022

Ok, in that case this could be accommodated. It might be a nice test of the system too, if you make everything in comoving and convert it to physical densities too... I'd like to see good test coverage here if this feature is added.

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

3 participants