Adaptahop automatic detection properties #160
Conversation
tangos/input_handlers/ramsesHOP.py
Outdated
| except (IOError, RuntimeError): | ||
| return False | ||
|
|
||
| def _exclude_adaptahop_precalculated_properties(self): |
There was a problem hiding this comment.
Misleading method name, as it doesn't do anything other than return a list?
tangos/input_handlers/ramsesHOP.py
Outdated
| patterns = ["output_0????"] | ||
| auxiliary_file_patterns = ["grp*.tag"] | ||
|
|
||
| def match_objects(self, ts1, ts2, halo_min, halo_max, dm_only=False, threshold=0.005, |
There was a problem hiding this comment.
Does this method copy/paste from somewhere else? It looks familiar. Is it possible to avoid the copy/paste and instead refactor to have this basic logic in one place?
There was a problem hiding this comment.
It resembles very much the same method defined in input_handlers/pynbody.py https://github.com/cphyc/tangos/blob/72e77fe8af987acf95e4d764ad0058e525bcbb0c/tangos/input_handlers/pynbody.py#L189-L212.
Note however that it has merely been moved from tangos/input_handlers/pynbody.py to tangos/input_handlers/ramsesHOP.py`.
tangos/input_handlers/ramsesHOP.py
Outdated
| "angular_momentum_x", "angular_momentum_y", "angular_momentum_z", | ||
| "contaminated", "m_contam", "mtot_contam", "n_contam", "ntot_contam"] | ||
|
|
||
| def _include_additional_properties_derived_from_adaptahop(self): |
There was a problem hiding this comment.
Misleading method name as above. By the way, why make these methods at all? You could just define them as static data within the class.
tangos/input_handlers/ramsesHOP.py
Outdated
| data = None | ||
|
|
||
| # Avoid naming confusions with already defined PynbodyProperties | ||
| if k == "shrink_center": data = (adaptahop_halo.properties['pos']).view(np.ndarray) |
There was a problem hiding this comment.
Is this definitely the same thing as a shrink_center? I don't know how adaptahop defines its halo positions. If it's basically the same, fine; otherwise shouldn't it be called something else to avoid confusion about the reliability of these centres?
There was a problem hiding this comment.
This is not the same in general as AdaptaHOP has different recipes to compute the centre, one of them being using a shrinking sphere approach. Unfortunately, as far as I can tell, there is no way of knowing what method has been used for centring.
There was a problem hiding this comment.
I see, I thought the shrink_center was the only option for AdapataHOP. I guess we could call it an adaptahop_center and leave it to users to map it to a shrink_center (or else) within their own property module.
… from the available list. Not sure this is the best behaviour, creates a lot of duplicates and naming issues
…a slightly brute force solution, but writing operations to the database end up being a million times slower
Co-authored-by: Corentin Cadiou <corentin.cadiou@cphyc.me>
…halo and properties out of the property loop
…d mapping to pure numpy view for storage
…fault which are now fast
cphyc
force-pushed
the
adaptahop-automatic-detection-properties
branch
2 times, most recently
from
1a2129d to
7f4b9f8
Compare
cphyc
force-pushed
the
adaptahop-automatic-detection-properties
branch
from
6e454ca to
1800c99
Compare
cphyc
force-pushed
the
adaptahop-automatic-detection-properties
branch
from
1800c99 to
7a4b462
Compare
|
@apontzen I think after further testing and cleaning up, @cphyc and I are now happy with this pull request (and the accompanying pynbody pull request for AdapatHOP https://github.com/pynbody/pynbody/pull/630/files). As far as we can tell, the handler behaves as expected, allows the import of properties in about ~5min per snapshot for a typical modern zoom, and support linking and merger tree building. Feel free to merge unless you have further comments! |
|
Thanks! |
Hi @apontzen,
This PR codes up a new
PynbodyHandlerfor AdaptaHOP halo and sub halo catalogues. It imports most of the pre-computed properties by AdaptaHOp (such as the virial mass or center), through a slightly different logic than usual, as unlike other handlers, these properties are stored as a dictionary attached to each pynbody halo rather than in a stat file.The handler also creates links between parent and child halos to navigate the relationship between halo and sub halos, although it currently only supports links between the first parent and its sub halos (i.e. subsub, subsubsubhalos etc are not tracked).
I am not entirely clear what the testing strategy is for new handlers, so far I have only verified carefully that the imported properties and links match between pynbody and Tangos. The import is quite fast on my local machine, taking about 100ms per halo to import all relevant quantities, which totals to ~5min per simulation output for a state-of-the-art zoom simulation.
In conjunction with https://github.com/pynbody/pynbody/pull/630/files and #159, this should provide a base pipeline for Tangos+pynbody+adaptaHOP significantly speeding up what we currently have.
Thanks to @cphyc for starting the base work and for reviewing the code. Any further comments welcome, this is the first time I touch this part of Tangos.
Best,
Martin