Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(5323)

Issue 52058: HOP/FOF changes

Can't Edit
Can't Publish+Mail
Start Review
Created:
14 years, 10 months ago by stephenskory
Modified:
3 months, 2 weeks ago
Reviewers:
matthewturk
CC:
yt-dev_lists.spacepope.org
Base URL:
http://svn.enzotools.org/yt/trunk/
Visibility:
Public.

Patch Set 1 #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+127 lines, -126 lines) Patch
M yt/lagos/HaloFinding.py View 2 chunks +2 lines, -1 line 2 comments Download
M yt/lagos/fof/EnzoFOF.c View 4 chunks +11 lines, -11 lines 0 comments Download
M yt/lagos/fof/fof_main.c View 2 chunks +10 lines, -10 lines 0 comments Download
M yt/lagos/fof/kd.h View 3 chunks +29 lines, -29 lines 1 comment Download
M yt/lagos/fof/kd.c View 17 chunks +56 lines, -56 lines 0 comments Download
M yt/lagos/hop/hop_regroup.c View 4 chunks +19 lines, -19 lines 0 comments Download

Messages

Total messages: 1
matthewturk
14 years, 10 months ago (2009-04-27 15:24:55 UTC) #1
This patch looks mostly alright to me -- thanks very much for getting rid of the
temporary file, and I agree that the FOF code should have a suffix to
distinguish it from HOP structures.  Here are some very minor comments...

http://codereview.appspot.com/52058/diff/1/2
File yt/lagos/HaloFinding.py (right):

http://codereview.appspot.com/52058/diff/1/2#newcode175
Line 175: _fields = ["particle_position_%s" % ax for ax in
'xyz',"particle_velocity_%s" % ax for ax in 'xyz']
This line is too long, split it to the next one.

http://codereview.appspot.com/52058/diff/1/2#newcode404
Line 404: self.data_source.get_data(["particle_velocity_%s" % ax for ax in
'xyz'])
This is redundant with the line above; additionally, we don't need
particle_velocity until the write_out.

http://codereview.appspot.com/52058/diff/1/5
File yt/lagos/fof/kd.h (right):

http://codereview.appspot.com/52058/diff/1/5#newcode38
Line 38: } KDFOFNFOF;
I think this should be KDNFOF; maybe the search-n-replace was done out of order?
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b