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-co... 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... https://bitbucket.org/yt_analysis/yt/issue/422/ray-casting-in-cylindrical-co... https://bitbucket.org/yt_analysis/yt/issue/421/refactor-non-cartesian-geomet... https://bitbucket.org/yt_analysis/yt/issue/345/non-cartesian-geometry https://bitbucket.org/yt_analysis/yt/issue/205/periodicity -Matt
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? Is it possible to replace axis_name, axis_id, x_axis, and y_axis with only axis_names = ['x', ...]? - Casey On Wed, Aug 29, 2012 at 7:28 AM, Matthew Turk <matthewturk@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-co...
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...
https://bitbucket.org/yt_analysis/yt/issue/422/ray-casting-in-cylindrical-co...
https://bitbucket.org/yt_analysis/yt/issue/421/refactor-non-cartesian-geomet... 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@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
On Wed, Aug 29, 2012 at 10:43 AM, Casey W. Stark <caseywstark@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@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-co...
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...
https://bitbucket.org/yt_analysis/yt/issue/422/ray-casting-in-cylindrical-co...
https://bitbucket.org/yt_analysis/yt/issue/421/refactor-non-cartesian-geomet... 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@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
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@gmail.com> wrote:
On Wed, Aug 29, 2012 at 10:43 AM, Casey W. Stark <caseywstark@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@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-co...
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...
https://bitbucket.org/yt_analysis/yt/issue/422/ray-casting-in-cylindrical-co...
https://bitbucket.org/yt_analysis/yt/issue/421/refactor-non-cartesian-geomet...
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@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
Hi Casey, I've updated the PR: https://bitbucket.org/yt_analysis/yt-3.0/pull-request/5/initial-import-of-co... 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@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@gmail.com> wrote:
On Wed, Aug 29, 2012 at 10:43 AM, Casey W. Stark <caseywstark@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@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-co...
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...
https://bitbucket.org/yt_analysis/yt/issue/422/ray-casting-in-cylindrical-co...
https://bitbucket.org/yt_analysis/yt/issue/421/refactor-non-cartesian-geomet... 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@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
Hey Matt. Don't worry about removing it on account of my tastes, especially when you've already written it! - Casey On Wed, Aug 29, 2012 at 8:27 AM, Matthew Turk <matthewturk@gmail.com> wrote:
Hi Casey,
I've updated the PR:
https://bitbucket.org/yt_analysis/yt-3.0/pull-request/5/initial-import-of-co...
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@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@gmail.com> wrote:
On Wed, Aug 29, 2012 at 10:43 AM, Casey W. Stark <caseywstark@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@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-co...
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...
https://bitbucket.org/yt_analysis/yt/issue/422/ray-casting-in-cylindrical-co...
https://bitbucket.org/yt_analysis/yt/issue/421/refactor-non-cartesian-geomet...
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@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
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(). 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. 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@gmail.com>wrote:
Hi Casey,
I've updated the PR:
https://bitbucket.org/yt_analysis/yt-3.0/pull-request/5/initial-import-of-co...
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@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@gmail.com> wrote:
On Wed, Aug 29, 2012 at 10:43 AM, Casey W. Stark <caseywstark@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@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-co...
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...
https://bitbucket.org/yt_analysis/yt/issue/422/ray-casting-in-cylindrical-co...
https://bitbucket.org/yt_analysis/yt/issue/421/refactor-non-cartesian-geomet...
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@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
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-co... and responded to your suggestions inline. On Wed, Aug 29, 2012 at 12:00 PM, Anthony Scopatz <scopatz@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@gmail.com> wrote:
Hi Casey,
I've updated the PR:
https://bitbucket.org/yt_analysis/yt-3.0/pull-request/5/initial-import-of-co...
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@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@gmail.com> wrote:
On Wed, Aug 29, 2012 at 10:43 AM, Casey W. Stark <caseywstark@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@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-co...
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...
https://bitbucket.org/yt_analysis/yt/issue/422/ray-casting-in-cylindrical-co...
https://bitbucket.org/yt_analysis/yt/issue/421/refactor-non-cartesian-geomet...
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@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
Great! I am glad it was helpful. Be Well Anthony On Fri, Aug 31, 2012 at 6:05 AM, Matthew Turk <matthewturk@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-co...
and responded to your suggestions inline.
On Wed, Aug 29, 2012 at 12:00 PM, Anthony Scopatz <scopatz@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
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
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
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@gmail.com> wrote:
Hi Casey,
I've updated the PR:
https://bitbucket.org/yt_analysis/yt-3.0/pull-request/5/initial-import-of-co...
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@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
but I think it ends up making things harder.
- Casey
On Wed, Aug 29, 2012 at 7:52 AM, Matthew Turk <matthewturk@gmail.com> wrote:
On Wed, Aug 29, 2012 at 10:43 AM, Casey W. Stark <caseywstark@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@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-co... > > 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
> 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
unmanageable. thing, like 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... > > > > https://bitbucket.org/yt_analysis/yt/issue/422/ray-casting-in-cylindrical-co... > > > > https://bitbucket.org/yt_analysis/yt/issue/421/refactor-non-cartesian-geomet... > > 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@lists.spacepope.org > http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
participants (3)
-
Anthony Scopatz -
Casey W. Stark -
Matthew Turk