search

pelahi / VELOCIraptor-STF

Galaxy/(sub)Halo finder for N-body simulations
MIT License
19 stars 26 forks source link

VR output field naming convention for multi-column arrays #74

Closed james-trayford closed 4 years ago

james-trayford commented 4 years ago

The current naming behavior for custom Gas_internal_property_ fields in VR is generally to add tags according to various properties, including the index of the column being aggregated over.
However in the exceptional case of the 0th column, there is no index_0 tag, the tag itself is left out. I think keeping an index_0 tag in these cases would be more intuitive, and clearer as to what the property is referring to. This avoids the need for special cases in the codes that read in the VR output. This of course is only needed if the input array is multi-column (e.g. ElementMassFractions in SWIFT runs)

pelahi commented 4 years ago

The issue is that you can load arrays that are not multicolumn (VR doesn't know beforehand how many columns there are). So even for single column data sets you would have index_0. Is there a preference for always having index_0?

james-trayford commented 4 years ago

ideally, it would only have the index_0 tag on multicolumn arrays (as ofc they are superfluous for 1D arrays), but there's some technical reason why VR can't tell if an array is 1D or 2D?

pelahi commented 4 years ago

It would just require a bit more coding so have VR update the output field name used based on the dimensions of the input data. The dimensions are currently not checked. I can try updating this

james-trayford commented 4 years ago

I think that would be great if it's not too much work, it sounds like this would work nicely with @JBorrow 's python library for VR.

pelahi commented 4 years ago

Now updated in the development branch https://github.com/pelahi/VELOCIraptor-STF/commit/a7676260bc6e06951ad6d613890f643e6e4f5841 @wmfw23, can you please test the code to see if the updated output names are as desired.

pelahi commented 4 years ago

I have now pushed these changes to master (along with fixing some MPI issues related to these extra fields). I have tested the code loading extra properties from an EAGLE snapshot. Results for properties like BH_Masses seem sensible. @wmfw23 and @JBorrow , can you please have a look at running the latest master on eagle and colibre data and see if the output data set names easily integrate with the python tools?

pelahi commented 4 years ago

I think this has been addressed.