search

ytree-project / ytree

A yt-based merger-tree code.
Other
15 stars 10 forks source link

Treefrog frontend fixes #95

Closed brittonsmith closed 3 years ago

brittonsmith commented 3 years ago

PR Summary

For comments on PR #88.

PR Checklist

manodeep commented 3 years ago

Thanks for your patience - I am testing this out now. Will report back

manodeep commented 3 years ago

I can report back on the UX for reading these treefrog files - everything seems to work fine. I followed instructions from the previous issue on adding ctrees-hdf5 format, and plotted some trees all worked fine. I have some minor thoughts that I have noted down.

Enhancement

Minor comments:

Would it be possible to provide a more helpful message by saying “You have provided the raw data file but I am expecting the foreststats file” or just helpfully inferring the foreststats filename and then proceeding as usual (perhaps with an info message that this was done)?

Unrelated issue:

The docs https://ytree.readthedocs.io/en/latest/Arbor.html#getting-started-with-merger-trees do not reflect what I see. For example, with 

In [10]: a.box_size
Out[10]: unyt_quantity(75.00008132, 'Mpc/h')

but the docs say

>>> print (a.box_size)
100.0 Mpc/h
brittonsmith commented 3 years ago

Enhancement

  • is it possible to add access=’forest’ or access=‘trees’ (just like for ctrees-hdf5?)

I'm not sure about how to do this efficiently. In ctrees-hdf5, the information analogous to file offsets and tree/forest sizes are stored for both forests and trees. We just have to change where to look. As far as I can tell, this information is only stored for forests in TreeFrog. It would be necessary to go through each forest manually and pull out all root halos. It's not undoable, but would be more than simply changing what data to read. Am I missing something? If not, I'm happy to create an enhancement issue for it and try to do it sometime in the future.

Minor comments:

  • If the user provides the forestid file (rather than the foreststats file), then ytree throws an error - “OSError: Could not determine arbor type for VELOCIraptor.tree.t4.0-131.walkabletree.sage.forestID.hdf5.0.”

I'll see what I can do about this. It's tricky because we support many different formats using HDF5 data. It takes being able to distinguish a file is of a specific format while not the correct file for that format.

Unrelated issue:

The docs https://ytree.readthedocs.io/en/latest/Arbor.html#getting-started-with-merger-trees do not reflect what I see. For example, with

In [10]: a.box_size
Out[10]: unyt_quantity(75.00008132, 'Mpc/h')

but the docs say

>>> print (a.box_size)
100.0 Mpc/h

What you're seeing here is the difference between the __str__ and __repr__ implementations for the unyt_quantity class in unyt. You get __str__ when you use print explicitly and __repr__ when you don't.

Thanks for the feedback. Rather than continuing to iterate on this PR, I will merge this and open issues for the enhancements you requested. We can continue to discuss there.

manodeep commented 3 years ago

Thanks for the explanation - and yup sounds good.

Thanks again for implementing this format :)