update starlog #491
update starlog #491
Conversation
…le (tcoolform)in starlog
pynbody/snapshot/tipsy.py
Outdated
| @@ -1204,9 +1228,9 @@ def __init__(self, filename, sort=True, paramfile=None): | |||
| 'f8', 'f8', 'f8', | |||
| 'f8', 'f8', 'f8', | |||
| 'f8', 'f8', 'f8')}) | |||
| moledH = False | |||
There was a problem hiding this comment.
Why is this moledH?
| moledH = False | |
| moledC = False |
|
This PR is a bit old, but can it still be incorporated? We're working with some simulations that have |
|
Thanks for the ping on this @emapple. I've updated the branch to be up to date with pynbody's master branch. I've also fixed the bug that @apontzen pointed out above. Sorry for having this fall off the radar. Please try out this pull request branch and let me know if you have issues. In particular, I'm not sure if this will play nice with 64 bit integer Iords. |
There was a problem hiding this comment.
@mtremmel I have included two specific changes. With them, I think it plays well with 64-bit iords. I'll note this isn't totally consistent with recent commits to pynbody. Namely, a recent PR adds an attempt to read in starlog metadata from the log file before. I suspect changes in that commit conflict with changes in this one, so it'll take some care to make sure the conflicts are resolved properly.
pynbody/snapshot/tipsy.py
Outdated
| @@ -943,6 +943,8 @@ def read_starlog(self, fam=None): | |||
| #b(sl)['velform'] = sl['vel'][:len(self.star), :] | |||
| if 'h2form' in sl.star.keys(): | |||
| b(b(b(sl))).star['h2form'] = b(b(sl)).star['h2form'] | |||
| if 'tcoolform' in sl.star.keys(): | |||
| b(b(b(sl))).star['tcoolform'] = b(b(sl)).star['tcoolform'] | |||
| else: print "No H2 data found in StarLog file" | |||
There was a problem hiding this comment.
I think this else statement is supposed to be above the 'tcoolform' if statement.
There was a problem hiding this comment.
I think I should just remove these print statements all together... plus they aren't compatible with python3 anyway as they are now I don't think
There was a problem hiding this comment.
@apontzen previously asked me to convert all of these prints to logger.info, but I haven't gotten around to it yet
There was a problem hiding this comment.
they weren't necessary so I removed them.
pynbody/snapshot/tipsy.py
Outdated
| "x", "y", "z", | ||
| "vx", "vy", "vz", | ||
| "massform", "rhoform", "tempform", "h2form"), | ||
| 'formats': ('i4', 'i4', 'f8', |
There was a problem hiding this comment.
This branch failed on a read of 64-bit iords for me, and I think it's because this is supposed to introduce 64-bit iords here, and uses 32-bit by mistake. Making the change from i4 to i8 fixed the problem for me locally.
If tcool == True, that means iSize is 104 (4*2 + 8*12) above, but here you've defined file_structure with a size of 96 (2*4 + 8*11). Therefore, here we should have 64-bit iords. This is consistent with the later logger info as well.
There was a problem hiding this comment.
I think it is a bit more complicated than that given all the degeneracies. I've tried to implement a change to fix this and allow for both h2 and tcoolform information with 64-bit iords.
There was a problem hiding this comment.
That update seems to work.
I thought tcoolform was phased out, and didn't realize there could be both 64-bit iords and tcoolform. I think this effectively solves the problem (though again, I think using the metadata in the log file is ultimately the best solution going forward).
There was a problem hiding this comment.
it is now, but might not be in your runs. This will hopefully ensure backwards compatibility. The problem is that due to the degeneracy, you may have a mismatch in manes (i.e. tcoolform may be marked as h2form). I also agree, I think this pull request will be unnecessary once we get a good metadata system going (assuming we can convert pre-existing files too)
in order to handle the possible inclusion of an extra data column in the starlog file (tcoolform).