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

default comoving attribute of cosmo_array should be None? #168

Open
kyleaoman opened this issue Sep 21, 2023 · 0 comments
Open

default comoving attribute of cosmo_array should be None? #168

kyleaoman opened this issue Sep 21, 2023 · 0 comments
Assignees

Comments

@kyleaoman
Copy link
Member

If we initialize a cosmo_array like this:

cosmo_array(np.array([1]) * u.Mpc)

the comoving attribute defaults to True. If we try to convert to physical units we get an exception because it tries to reference attributes of the cosmo_factor which is None, but trying to convert to comoving works fine. This seems dangerous, wouldn't it be better to default to None when the situation is ambiguous? Alternatively returning a unyt_array instead wouldn't be unreasonble, but is probably more confusing to someone expecting a comso_array. Could do it anyway and produce a warning.

Implementing this would need some checking that nothing in swiftsimio relies on the default. Test coverage might be good enough for issues to raise their head if it is relied on, but then again they might not ;)

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

2 participants