[yt-dev] ellipsoid pull request suggestions from Matt

[Matthew Turk]

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
>