yt-project / yt

Main yt repository
http://yt-project.org
Other
469 stars 280 forks source link

BUG: Off-axis projection plots do not respect units of derived fields #4666

Open clochhaas opened 1 year ago

clochhaas commented 1 year ago

Bug report

Bug summary

When making off-axis projection plots using a user-defined velocity field in units of km/s, the plot gives values that are in cm/s instead of km/s and setting the units for the field in the projection plot doesn't change the behavior.

Code for reproduction

This code uses the enzo_tiny_cosmology public dataset.

import yt

ds = yt.load('/Users/clochhaas/Downloads/enzo_tiny_cosmology/DD0046/DD0046')

# Define a velocity field with a simple shift in the x-velocity as an example
def shift_velocity_x(field, data):
    return data['gas','velocity_x'] - ds.quan(30., 'km/s')

# Add the field with units='km/s'
ds.add_field(('gas', 'shift_velocity_x'), function=shift_velocity_x, units='km/s', take_log=False, force_override=True, sampling_type='cell')

# This one works as expected, with values and units in km/s
proj = yt.ProjectionPlot(ds, [1,0,0], ('gas','shift_velocity_x'), weight_field=('gas','density'))
proj.save('axis-aligned_user-defined-field.png')

# This one prints the units as km/s but is clearly in cm/s, even after setting the units manually
proj = yt.ProjectionPlot(ds, [0.95,0.05,0], ('gas','shift_velocity_x'), weight_field=('gas','density'))
proj.set_unit('shift_velocity_x', 'km/s')
proj.save('off-axis_user-defined-field.png')

# This one uses the default Enzo velocity_x field and works as expected even though it is off-axis
proj = yt.ProjectionPlot(ds, [0.95,0.05,0], ('gas','velocity_x'), weight_field=('gas','density'))
proj.set_unit('velocity_x', 'km/s')
proj.save('off-axis_default-field.png')

# If the user-defined field is defined with units='cm/s', then everything works as expected:
ds.add_field(('gas', 'shift_velocity_x'), function=shift_velocity_x, units='cm/s', take_log=False, force_override=True, sampling_type='cell')

# This clearly has correct units even though it is off-axis with the user-defined field
proj = yt.ProjectionPlot(ds, [0.95,0.05,0], ('gas','shift_velocity_x'), weight_field=('gas','density'))
proj.set_unit('shift_velocity_x', 'km/s')
proj.save('off-axis_user-defined-field-cm.png')

Actual outcome

axis-aligned_user-defined-field.png: axis-aligned_user-defined-field

off-axis_user-defined-field.png: off-axis_user-defined-field

off-axis_default-field.png: off-axis_default-field

off-axis_user-defined-field-cm.png: off-axis_user-defined-field-cm

Expected outcome

The expected outcome is that the second image above (off-axis_user-defined-field.png) should look the same as the fourth image (off-axis_user-defined-field-cm.png), rather than having values on the colorbar that are 5 orders of magnitude larger. The only difference between these two is that the user-defined field is given different units in the call to ds.add_field.

Version Information

yt and python were both installed from conda, using the conda-forge channel.

welcome[bot] commented 1 year ago

Hi, and welcome to yt! Thanks for opening your first issue. We have an issue template that helps us to gather relevant information to help diagnosing and fixing the issue.

neutrinoceros commented 1 year ago

Thanks @clochhaas for reporting this in such details ! I tried to reproduce this with a simple test dataset

from yt.testing import fake_amr_ds
ds = fake_amr_ds(fields=[("gas", "density"), ("gas", "velocity_x")], units=["g/cm**3", "cm/s"])`)

and the behaviour seems correct there (there are potentially other issues, but not the ones you're seeing with Enzo data).

Therefore I suspect this may be a frontend-specific bug. I'll investigate further

neutrinoceros commented 1 year ago

So I discovered that there's an important difference between how units are determined for on-axis projections and off-axis ones when creating the underlying "image array" (a 2D buffer with units):

Unfortunately the "dirty no-good" hack doesn't work for off-axis projections, because a OffAxisProjectionDummyDataSource isn't subscriptable (it lacks support for __getitem__).

I think the bug is showing the limits of the pile of hacks that currently is the backbone of off-axis projections in yt. I'm quite out of my depth on this one, so I don't think I'll be able to quickly patch it as I initially hoped...

jzuhone commented 1 year ago

@clochhaas i cannot promise I will get to this before next week, but soon.