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

Clarification needed on the units #63

Closed
astronerdF opened this issue Aug 3, 2023 · 2 comments
Closed

Clarification needed on the units #63

astronerdF opened this issue Aug 3, 2023 · 2 comments

Comments

@astronerdF
Copy link

While loading the Coordinates of the gas particles using the Illustris package, the units given on the website is ckpc/h https://www.tng-project.org/data/docs/specifications/#parttype0
The units for GroupPos variable as well are ckpc/h as well.

When looking at the same variables using the scida package, the units for Coordinates are centimeter

coords = gas["Coordinates"]
coords.to_base_units().units
----------------------
</> centimeter

When looking at the GroupPos variable, the units are dimensionless

import dask.array as da
ds = load(basePath+"/snapdir_099", units=True)
data = ds.return_data(haloID=42)
grp = data["Group"]
grp["GroupPos"].units
----------------------
</> dimensionless

Additionally, for the variable Coordinates, the maximum value for the same snapshot (TNG100-3 snap 99) is the same whether the data is read from the Illustris package or the Scida package:

gas_il = il.snapshot.loadHalo(basePath,99,0,'gas',fields="Coordinates")
print("gas_il -  max Coordinate: ", np.max(np.array(gas_il))) #UNITS ckpc/h

ds = load(basePath+"/snapdir_099", units=True)
print("gas_scida - max Coordinate: ", ds.data["Group"]["Coordinates"].max().compute())
-----------------------------------------------------------------------------------------
</> gas_il -  max Coordinate:  74999.99978918549
 gas_scida - max Coordinate:  74999.8984375 dimensionless
@cbyrohl
Copy link
Owner

cbyrohl commented Aug 4, 2023

Thanks for pointing this out. "dimensionless" is currently also used if units cannot automatically be detected. TNG100-2/TNG100-3 were apparently not correctly detected. The "Coordinates" field has metadata allowing us to determine the units nonetheless. This metadata is unavailable for the group catalogs. Thus the missing data.

Upon load() there should have been a "WARNING:scida.interfaces.mixins.units: [...]" point this out?

In the future, if "a few" fields cannot have their units determined, we should set the unit to a custom "unknown" rather than "dimensionless". If all/more than "a few" fields do not have units, we should fall back to no units at all.

cbyrohl added a commit that referenced this issue Aug 7, 2023
@cbyrohl
Copy link
Owner

cbyrohl commented Aug 8, 2023

If not earlier, then with PR #69 TNG100-2/TNG100-3 are correctly detected. Furthermore, when we do not have unit information for a field, then the units are "unknown" rather than "dimensionless" from now on.

@cbyrohl cbyrohl closed this as completed Aug 8, 2023
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