[yt-dev] ellipsoid pull request suggestions from Matt

Matthew Turk matthewturk at gmail.com
Fri Jun 22 21:10:15 PDT 2012 

Hi Geoffrey,

I've left a comment on the PR, but I liked the changes you have made.
I'll replace your calls to RX,RY,RZ with direct calls to the rotation
matrix functions after it's been accepted.

The last thing -- Sam asked for some plots so we can see this in
action.  I think this is pretty reasonable, and would help us
experiment a bit.  Nathan and I chatted and we decided that the
orientation stuff can be added later as well, so looks like it's good
to go.

Thanks again for taking the time to read our comments and update your
pull request.

On Fri, Jun 22, 2012 at 3:43 PM, Geoffrey So <gsiisg at gmail.com> wrote:
> This is contiuing a discussion where Nathan suggested that I imitate what
> was done in the orientation.py class object
>
> Hi Nathan,
>
> I took a quick look at orientation.py and
> yt/visualization/volume_rendering/camera.py (I think that's what you meant),
> and I think it might be a lot of work to take out the current code and put
> it into an orientation class, at least with my familiarity level with
> python.  So unless there's a strong need for it (or unless this can be
> simply done in a couple of minutes), I'm leaning towards getting this in as
> is, and maybe change it at a later date.  Primarily I'm afraid that bugs
> will be intruduced and debugging will take a while (YT 2.X deadline
> looming).
>
> I will of course add this to the list of possible future development though.
>
> -----------------------------------------------------------------------------------------------------------------------------------------
>
> Continue on this pull request topic,
>
> Hi Matt,
>
> On Rotations:
> You said I should use the get_rotation_matrix, I defined RX,Y,Z in
> math_utils.py and made sure it reproduced answers correctly.  I was
> wondering should I just define RX using get_rotation_matrix?  This way the
> code doesn't have to change, or should I replace calls to RX(theta) with
> call to get_rotation_matrix(-theta,(1,0,0)) etc?
>
> On Class:
> Currently in halo_objects.py the get_ellipsoid_parameters_basic() is already
> defined under the "class Halo(object)".  There are some functions that call
> get_ellipsoid_get_parameters_basic for different types of Halo finder.  I
> think this is because Stephen tried to get this working inline as the Halo
> finder (HOP, FOF, or parallelHOP) is run, and include the ellipsoid
> attributes in the halo finder output file, but he couldn't get it working
> for parallelHOP for the inline part and I'm not too sure how to tinker with
> them.  The AMREllipsoidBase stuff in data_containers.py are not the
> ellipsoidal functions you're referring to right?
> From
> G.S.
>
> On Thu, Jun 21, 2012 at 3:21 AM, Matthew Turk <matthewturk at gmail.com> wrote:
>>
>> Hi Geoffrey,
>>
>> For everyone else out there, the pull request is here:
>>
>> https://bitbucket.org/yt_analysis/yt/pull-request/173/ellipsoid-fork
>>
>> This provides some context to this discussion.  Also, Geoffrey, Sam
>> left a comment on the PR that I think would also be a good addition.
>>
>> On Wed, Jun 20, 2012 at 9:03 PM, Geoffrey So <gsiisg at gmail.com> wrote:
>> > Hi Matt, thanks for the suggestions you've made on the pull request,
>> > there's
>> > just a couple questions and technical terms that I didn't understand.
>>
>> Okay.  I will do my best to go over them.  One of the reasons we have
>> pull requests is to iterate on ideas and code, and for a feature that
>> adds a good chunk of code, we want to make sure that code is
>> maintainable as well as clear.
>>
>> > - What is a base class or subclasses in YT?  Where can I avoid the code
>> > reuse, and override?
>>
>> One of the ways that Python operates is through hierarchical
>> definition of objects.  Below I've given a ten-cent tour of this, but
>> a much better place to go would be the Inheritance section of this
>> document: http://docs.python.org/tutorial/classes.html
>>
>> For instance, we might have something like this:
>>
>> class Person:
>>    def congratulate(self):
>>        print "Thank you."
>>
>>    def greet(self):
>>        print "Hi there!"
>>
>> Now whenever you instantiate a Person, it will have the method
>> "greet".  For instance:
>>
>> matt = Person()
>> matt.greet()
>>
>> will print out "Hi there!".  Let's say that we then wanted to add
>> specialization:
>>
>> class FriendlyPerson(Person):
>>    def greet(self):
>>        print "It's good to see you!"
>>
>> Now we can do this:
>>
>> fp = FriendlyPerson()
>> fp.greet()
>>
>> and it will print out "It's good to see you!".  But because it's a
>> *subclass* of Person, we also get access to all the methods of the
>> base class, Person.  So we can congratulate it too:
>>
>> fp.congratulate()
>>
>> and it reverts to the base class.  What the halos are set up to do is
>> to share a number of similar methods and routines, but to specialize
>> where necessary.  I am suggesting you move the ellipsoidal functions
>> into the base class ("Halo") that all the halo types subclass from and
>> specialize.
>>
>> This naturally avoids code reuse by reducing the number of places
>> identical code is included, which helps maintainability and clarity.
>>
>> > - I see inside math_utils.py already has an get_rotation_matrix at the
>> > end,
>> > which is probably the general case you're referring to earlier when we
>> > talked (not sure how to use that with the "self" in the input).  I'll
>> > just
>> > stick in the less general case of RX, RY, RZ unless you think it's
>> > redundant.
>>
>> I had forgotten there was a rotation matrix function, which it looks
>> like was put in by Nathan for the camera orientation.
>>
>> Your implementation currently defines six functions:
>>
>> RX, RY, RZ in the halo module
>> RX, RY, RZ in the scope of the initializer function in the ellipsoid.
>>
>> Defining functions in the scope of other functions is in general
>> somewhat frowned upon unless they require "closure" information from
>> the surrounding scope, but more to the point you have defined these
>> functions that perform the same operation in two different places.
>> They are identical functions, too.  So what happens if we discover a
>> bug?  It has to be fixed two places.  That was the initial impetus for
>> my saying that you should move them into math_utils.py, so that we can
>> both put them somewhere they could be used by other things, but also
>> so that we could put them in a single place to be fixed.
>>
>> But now that it looks like Nathan's rotation matrix functions are
>> there, I think the preferred solution would be to remove the
>> definitions of RX, RY and RZ and instead replace them with a single
>> call to get_rotation_matrix.
>>
>> After a quick set of tests, it looks to me like the only difference
>> between get_rotation_matrix's functionality and RX,RY,RZ is that the
>> convention for theta is reversed; your theta is negative the one used
>> there.  After investigating this, I think you should absolutely use
>> the rotation matrix function that's already in yt, and remove RX, RY
>> and RZ from both data_containers.py and the halo file.
>>
>> Also, I've fixed the get_rotation_matrix 'self' argument.  Not sure
>> how that slipped in.
>>
>> >
>> > * Instead of printing, I would suggest either raising an exception
>> > (which
>> > you can define in yt/exceptions.py) or issuing a warning with
>> > {{{mylog.warning("Not enough particles to determine ellipsoidal
>> > parameters.")}}}
>> >
>> > - What is better?  Should I do a warning message and let the user know
>> > I'm
>> > returning zeros for the ellipsoid parameters?  Or raise an exception,
>> > then
>> > let the user decide what to do if an exception is raised?
>>
>> I am slightly more in favor of the warning, since I think a 4-particle
>> halo is a ridiculous corner case.  (Do any of our halo finders even
>> return this, unless specifically asked for it?  I seriously hope not.)
>>
>> > - I think I tried to write an exception but didn't quite understand how
>> > it
>> > works, going to give it another try here
>> >
>> > Do I replace
>> >
>> >         if na.size(self["particle_position_x"]) < 4:
>> >             print "not enough particles to form ellipsoid returning
>> > zeros"
>> >             return (0, 0, 0, 0, 0, 0, 0)
>> >
>> > with something like
>> >
>> >         if na.size(self["particle_position_x"]) < 4:
>> >             raise
>> > YTNotEnoughParticles(na.size(self["particle_position_x"]))
>> >
>> > and in exceptions.py
>> >
>> >         class YTNotEnoughParticles(YTException):
>> >             def __init__(self, nparticles):
>> >                 self.nparticles = nparticles
>> >
>> >             def __str__(self):
>> >                 return "There are only %i particles, not enough to form
>> > an
>> > ellipsoid" % (self.nparticles)
>> >
>>
>> Yup, spot on.
>>
>> -Matt
>>
>> > From
>> > G.S.
>> >
>> > _______________________________________________
>> > yt-dev mailing list
>> > yt-dev at lists.spacepope.org
>> > http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
>> >
>> _______________________________________________
>> yt-dev mailing list
>> yt-dev at lists.spacepope.org
>> http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
>
>
>
> _______________________________________________
> yt-dev mailing list
> yt-dev at lists.spacepope.org
> http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
>

More information about the yt-dev mailing list