[yt-dev] Coordinate handling pull request

[Anthony Scopatz]

Great!  I am glad it was helpful.

Be Well
Anthony

On Fri, Aug 31, 2012 at 6:05 AM, Matthew Turk <matthewturk at gmail.com> wrote:

> Hi Anthony,
>
> Thanks for your suggestions!  I've updated the pull request here:
>
>
> https://bitbucket.org/yt_analysis/yt-3.0/pull-request/5/initial-import-of-coordinate-handler-class
>
> and responded to your suggestions inline.
>
> On Wed, Aug 29, 2012 at 12:00 PM, Anthony Scopatz <scopatz at gmail.com>
> wrote:
> > Hello Matt,
> >
> > Thanks for putting this together!  After briefly looking this over, it
> seems
> > like some methods which have "cartesian" in the name are slightly
> mis-named.
> > As implemented, cartesian_length() is really just a path_length().
>
> Ah, alright.  I've changed the name to path_length, and cleaned up a
> few other methods.
>
> >
> > I agree with the sentiment of the convert_to/from_cartesian() methods.
> > However, I think half of these could be redundant.  It basically depends
> on
> > whether you are choosing that Cartesian be the canonical form or not.
>  So I
> > see two strategies:
> >
> > 1) All other systems have to/from cartesian methods.  Only need two
> methods
> > per coord system.  All coord transformations go through this cartesian.
> > (What you have, cleaner structure).
> >
> > 2) All systems have either a to_<othersystem>() method.  One method per
> > conversion you wish to support.  Could be faster and have less floating
> > point error than #1.  This prevents the situation where to go from Polar
> ->
> > Spherical you actually have to do Polar -> Cartesian -> Spherical.
> >
> >
> > The disadvantage of #2 is that you have to write more methods.  I guess
> > nothing is stopping you from implementing #1 and then just tacking on
> these
> > extra methods from #2 for cases where it makes sense.
>
> On thinking about it, I like #2 better as well.  I'm going to start
> with just cylindrical and cartesian, and I've gone ahead and
> implemented the to/from methods between cartesian and cylindrical.
>
> If everyone likes this, I'll continue on adding fields for it and
> looking at how to integrate coordinate handling in a deeper way.  Let
> me know if there are any other suggestions.
>
> -Matt
>
> >
> > My initial thoughts are that there are only 4 or 5 of these coordinate
> > systems so the number of permutations is not so high as to be
> unmanageable.
> > I also doubt that this is the kind of thing that users will be
> subclassing
> > and creating their own versions of.  So this is basically a "write once"
> > piece of code (::crosses fingers::).
> >
> > In any event, I think what you have now is great.  It would just be nice
> to
> > circumvent going through cartesian when that is clearly too much work.
> >
> > Be Well
> > Anthony
> >
> > PS Sorry if this was rambling.  Must. find. coffee.
> >
> > On Wed, Aug 29, 2012 at 10:27 AM, Matthew Turk <matthewturk at gmail.com>
> > wrote:
> >>
> >> Hi Casey,
> >>
> >> I've updated the PR:
> >>
> >>
> >>
> https://bitbucket.org/yt_analysis/yt-3.0/pull-request/5/initial-import-of-coordinate-handler-class
> >>
> >> I think for now I'm going to leave the ABC in as a bookkeeping
> >> exercise while implementing the other handlers, and remove it later.
> >>
> >> -Matt
> >>
> >> On Wed, Aug 29, 2012 at 11:18 AM, Casey W. Stark <caseywstark at gmail.com
> >
> >> wrote:
> >> > Hey Matt.
> >> >
> >> > Sounds good. I would guess the dimensionality issue comes down to
> >> > implementation, but I think subclasses are fine.
> >> >
> >> > I am -1 on using abstract classes in general. Completely a style
> thing,
> >> > but
> >> > I think it ends up making things harder.
> >> >
> >> > - Casey
> >> >
> >> >
> >> > On Wed, Aug 29, 2012 at 7:52 AM, Matthew Turk <matthewturk at gmail.com>
> >> > wrote:
> >> >>
> >> >> On Wed, Aug 29, 2012 at 10:43 AM, Casey W. Stark
> >> >> <caseywstark at gmail.com>
> >> >> wrote:
> >> >> > Hey Matt.
> >> >> >
> >> >> > I think this would be a big improvement, but I was wondering how it
> >> >> > interacts with other yt pieces. Does each output have geometry and
> >> >> > coordinate_handler objects as attributes?
> >> >>
> >> >> Yup, that is the plan.  The idea is that we move all of the IO and
> >> >> particle/fluid selection into the geometry handler, and the handling
> >> >> of spatial layout to the coordinate handler.  This would mean, for
> >> >> instance, that we could push periodicity as well as path length into
> >> >> the coordinate handler; this would remove some of the need to
> >> >> constantly do wraparound checks and the like.
> >> >>
> >> >> There is still somewhat the issue that *selection* of points to
> >> >> understand coordinate systems, which will require a bit more thought
> >> >> in the future but I think is still a tractable problem.
> >> >>
> >> >> >
> >> >> > Is it possible to replace axis_name, axis_id, x_axis, and y_axis
> with
> >> >> > only
> >> >> > axis_names = ['x', ...]?
> >> >>
> >> >> It is, but not with abc.abstractproperty.  (Initially I figured the
> >> >> cost of creating the dicts was low enough that we could do this to
> >> >> avoid worrying about mutable, class-level properties, but I think
> >> >> perhaps I like yours better.)  I'll remove some of the fancier
> >> >> ABC-stuff and slim it down.
> >> >>
> >> >> >
> >> >> > - Casey
> >> >> >
> >> >> >
> >> >> > On Wed, Aug 29, 2012 at 7:28 AM, Matthew Turk <
> matthewturk at gmail.com>
> >> >> > wrote:
> >> >> >>
> >> >> >> Hi all,
> >> >> >>
> >> >> >> I've issued a pull request to the 3.0 repository, as I think it
> >> >> >> warrants discussion.  It's here:
> >> >> >>
> >> >> >>
> >> >> >>
> >> >> >>
> >> >> >>
> https://bitbucket.org/yt_analysis/yt-3.0/pull-request/5/initial-import-of-coordinate-handler-class
> >> >> >>
> >> >> >> This includes a first pass at a coordinate handling system.  This
> is
> >> >> >> distinct from a geometry handling system; the coordinates here
> refer
> >> >> >> to how we handle coordinates and spatial locations internally,
> >> >> >> whereas
> >> >> >> geometry refers to how data is distributed throughout a domain and
> >> >> >> throughout places on disk.  For instance, coordinate handling
> would
> >> >> >> be
> >> >> >> cartesian, polar, spherical.
> >> >> >>
> >> >> >> The reason I'm bringing it up for discussion is that I believe we
> >> >> >> want
> >> >> >> to move as much coordinate handling and transformation into a
> >> >> >> separate, well-defined class as possible.  Periodicity, distances
> >> >> >> and
> >> >> >> so on are all currently scattered throughout the code, and I'd
> like
> >> >> >> to
> >> >> >> try to consolidate them.  Additionally, as new coordinate systems
> >> >> >> (polar, spherical) are added, we'll need clear ways to delegate
> >> >> >> responsibility for things like "How do I calculate path length as
> I
> >> >> >> integrate?" or "What's the way to turn this into an image?"  I
> >> >> >> believe
> >> >> >> the best way to do that is to attach a coordinate system to the
> >> >> >> dataset object itself.  (We now have a polar pixelizer
> >> >> >> http://i.imgur.com/a4UGg.png !)
> >> >> >>
> >> >> >> The interface is currently set such that you need to define these
> >> >> >> methods and properties in order to implement a coordinate system:
> >> >> >>
> >> >> >> coordinate_fields (this may go away, but it's for the analogs of
> >> >> >> 'x',
> >> >> >> 'y', 'z', as well as volume)
> >> >> >> pixelize
> >> >> >> convert_from_cartesian
> >> >> >> convert_to_cartesian
> >> >> >> axis_name
> >> >> >> axis_id
> >> >> >> x_axis
> >> >> >> y_axis
> >> >> >> period
> >> >> >>
> >> >> >> Some of these currently live in dictionaries in
> >> >> >> yt/utilities/definitions.py, which is pretty sub-optimal.  I'd
> like
> >> >> >> to
> >> >> >> ask for feedback:
> >> >> >>
> >> >> >> 1) Do these methods sufficiently cover everything we need to know
> in
> >> >> >> yt about a coordinate system?  Should any be added?
> >> >> >> 2) Do we need to directly address dimensionality as a separate
> >> >> >> subclass?
> >> >> >> 3) Should any of these be removed?
> >> >> >>
> >> >> >> This will also help address these issues:
> >> >> >>
> >> >> >>
> >> >> >>
> >> >> >>
> >> >> >>
> https://bitbucket.org/yt_analysis/yt/issue/418/use-a-right-handed-coordinate-system
> >> >> >>
> >> >> >>
> >> >> >>
> >> >> >>
> https://bitbucket.org/yt_analysis/yt/issue/422/ray-casting-in-cylindrical-coordinates
> >> >> >>
> >> >> >>
> >> >> >>
> >> >> >>
> https://bitbucket.org/yt_analysis/yt/issue/421/refactor-non-cartesian-geometry
> >> >> >>
> >> >> >>
> https://bitbucket.org/yt_analysis/yt/issue/345/non-cartesian-geometry
> >> >> >> https://bitbucket.org/yt_analysis/yt/issue/205/periodicity
> >> >> >>
> >> >> >> -Matt
> >> >> >> _______________________________________________
> >> >> >> 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
> >> >
> >> >
> >> >
> >> > _______________________________________________
> >> > 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
> >
> _______________________________________________
> yt-dev mailing list
> yt-dev at lists.spacepope.org
> http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.spacepope.org/pipermail/yt-dev-spacepope.org/attachments/20120831/7516e90c/attachment.html>