<div dir="ltr">Hi Nathan,<div><br></div><div>After testing out your PR, I am strongly in favor of this.  I really like the fact that (for dd = pf.h.all_data()):</div><div>dd['metallicity']</div><div>and</div><div>dd['metal_density'] / dd['density']</div>
<div><br></div><div>are 100% consistent with each other in their values and units.</div><div><br></div><div>I think I may have been the person who originally added metallicity to the list of dimensions.  This was likely due to a lack of understanding of what needs to be done when adding a new unit.  The distinction between unit systems and dimensions is somewhat unclear.  It might be nice to have a short discussion of this in some developer documentation at some point.</div>
<div><br></div><div>Excellent job with this.</div><div><br></div><div>Britton</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Feb 6, 2014 at 2:44 PM, Nathan Goldbaum <span dir="ltr"><<a href="mailto:nathan12343@gmail.com" target="_blank">nathan12343@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi all,<br>
<br>
I've updated the metallicity units PR:<br>
<a href="https://bitbucket.org/MatthewTurk/yt/pull-request/38/fixing-cgs-unit-conversions-for-angle-and/diff" target="_blank">https://bitbucket.org/MatthewTurk/yt/pull-request/38/fixing-cgs-unit-conversions-for-angle-and/diff</a><br>

<br>
I ended up deciding that metallicity doesn't really make much sense as<br>
a dimension in the unit system.  Instead, I've made it so the<br>
`code_metallicity` and `Zsun` units are dimensionless.<br>
<br>
Here's an example run using the tip of the PR which illustrates why<br>
this is a nice choice:<br>
<br>
First, load the data:<br>
<br>
>>> pf = load('HiResIsolatedGalaxy/DD0044/DD0044')<br>
>>> dd = pf.h.all_data()<br>
<br>
Get the mean metal mass for all particles in the simulation:<br>
<br>
>>> mean_metal_mass = (dd['metallicity_fraction']*dd['particle_mass']).mean()<br>
>>> print mean_metal_mass<br>
4.13009892602e-09 code_mass*code_metallicity<br>
>>> print mean_metal_mass.in_cgs()<br>
8.21517977375e+33 g<br>
<br>
If I had run this using a version with a metallicity dimension, I<br>
would have had to coerce the units to be grams, even though code<br>
metallicity is really just metallicity fraction. I think getting a<br>
metal mass or density like this will be an extremely common operation,<br>
and it's something that a metallicity unit needs to handle naturally.<br>
<br>
Here's another example where I get the mean metallicity of all<br>
particles in the simulation:<br>
<br>
>>> mean_metallicity = dd['metallicity_fraction'].mean()<br>
>>> mean_metallicity.in_cgs()<br>
0.00503378562375 dimensionless<br>
>>> mean_metallicity<br>
0.00503378562375 code_metallicity<br>
>>> mean_metallicity.in_units('Zsun')<br>
0.246633298567 Zsun<br>
<br>
In principle code_metallicity can have a non-unit conversion to a<br>
dimensionless fraction, but enzo writes out particle metallicities as<br>
mass fractions, so that is unnecessary here.<br>
<br>
As another concrete example why this way is better, here is the way I<br>
needed to define the metal_density field for a Tipsy SPH dataset:<br>
<br>
@derived_field(name = "metal_density", units = "g/cm**3")<br>
def metal_density(field, data):<br>
    ret = data['metallicity_fraction']*data['density']<br>
    ret.convert_to_units('g/cm**3*code_metallicity')<br>
    ret = np.array(ret)<br>
    return data.pf.arr(ret, input_units='g/cm**3')<br>
<br>
If metallicity weren't a base dimension, I wouldn't need to coerce the<br>
units to g/cm**3, the conversion to CGS would handle that<br>
automatically.<br>
<br>
Hope that explains my position clearly.  I'd love to hear comments,<br>
concerns, or alternate approaches - this is definitely tricky.<br>
<br>
-Nathan<br>
_______________________________________________<br>
yt-dev mailing list<br>
<a href="mailto:yt-dev@lists.spacepope.org">yt-dev@lists.spacepope.org</a><br>
<a href="http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org" target="_blank">http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org</a><br>
</blockquote></div><br></div>