Particle position and velocity fields for cylindrical and spherical coordinates

#1378 Declined
Repository
ngoldbaum
Branch
yt
Repository
yt_analysis
Branch
yt
Author
  1. Nathan Goldbaum
Reviewers
Description

This PR is a replacement for @Benjamin Thompson's PR 1308. I did some big updates based on his work, and the discussion in that PR was starting to get really lengthy, so I've reissued the PR here.

This PR does two main things:

  1. The fields currently in yt with names like particle_spherical_position_radius return incorrect results. See #966. This PR ensures these fields return correct results.

  2. Adds new particle_position_relative and particle_velocity_relative fields. This makes it possible to get particle positions and velocities in a rotated reference frame.

  3. The naming system for particle vector fields has never been codified. In this PR I implement a proposed naming system. Particle vector fields will be of the form:

particle_<vector field name>_coordinate

The main advantage is that this matches our current naming system for cartesian coordinates (e.g. particle_position_x, particle_velocity_y. So the spherical radius position field becomes particle_position_spherical_radius.

Where there are fields that are currently in yt that have names that do not match this naming convention, I've stubbed out their implementation and made them aliases for the fields with names following my proposed naming convention. I've also marked these stub fields as deprecated in their docstrings.

When no field already existed (e.g. the position fields in cylindrical coordinates), I've simply added the new fields following my proposed naming convention without adding fields that would immediately need to be deprecated.

I'm confident that these are all giving correct results. See this notebook:

http://nbviewer.ipython.org/gist/ngoldbaum/e86250523d2ce2036669

Note that this notebook uses some work-in-progress machinery from PR 1377.

I'm not sure whether this should go in before 3.1. It's a significant change that includes some new code and fields, but it does correct some big issues with the spherical particle position fields. I would very much appreciate comments on that front from PR reviewers.

Comments (14)

      1. chummels

        Your proposed naming scheme for particle fields has direct impacts for corresponding gas fields.

        For example, by choosing to have particle fields as (as you do in this PR):

        particle_position_spherical_radius

        the equivalent gas field would be

        position_spherical_radius

        Whereas by choosing the particle fields to be (as was done previously, and suggested in the issue above):

        particle_spherical_position_radius

        the gas fields would be:

        spherical_position_radius

        Admittedly, both styles seem to be somewhat awkward constructions, but both gas fields and particle fields seemed to me to be slightly less awkward for the second set. That said, I'm open to changing it, I just wanted everyone to realize the consequences this proposed change would have for other fields.

            1. Nathan Goldbaum author

              OK, then the field names would be of the form:

              ('gas', 'velocity_spherical_radius')

              Which I think is preferable to

              ('gas', 'spherical_velocity_radius')

              for the same reasons I outlined in the PR description - mainly consistency with ('gas', 'velocity_x'), but also it seems more natural to me.

              1. chummels

                OK, so we disagree on the convention, which is fine, but I think other people should vote on the new proposed convention. Ultimately, I don't care that much, but I just want consistency between the various parts of the code so we don't end up with inconsistent field naming as we've sometimes had in the past as the code grew organically.

                1. Nathan Goldbaum author

                  What would it take to get you to approve this?

                  I can:

                  • Update the gas velocity fields to match this naming convention

                  • Update the field naming YTEP

                  • Update the index position fields to match this naming convention (already done, I'd argue).

                  1. chummels

                    I just wanted to get feedback from the other devs on which is the best naming convention to use between the two proposed. When there is general consensus, I am happy to approve this PR, since it adds new functionality, removes broken functionality, makes a consistent field naming scheme, and adds documentation.

  1. Benjamin Thompson

    I agree that the gas field names should be updated for consistency. In my opinion I like the convention of

    <particle?><vector_field_name><co-ordinate>

    under this convention, would mean changing particle_relative_position to particle_position_relative?

    Likewise for consistency, the gas fields would benefit from position_relative as well.

    I am also for the idea of some sort of vote which @chummels has suggested. But I don't think it would be a good idea to spend days discussing a field convention... Pick something and stick with it.