#2375 Merged at 472c5ba
  1. Nathan Goldbaum

This backports a number of bugfixes in preparation for yt 3.3.2.

I'm particularly appreciate it if @chummels or @brittonsmith could test this out on trident, since there are a number of absorption spectrum or light ray-specific bugfixes.

  • Commit status

Comments (6)

  1. Devin Silvia

    I just ran the Trident testing suite with this PR and, as far as I can tell, everything functions as intended.

  2. chummels

    OK, I've looked at this and done a bunch of testing with the stable version of Trident to see if this backport breaks anything. It turns out that the tests run fine in trident, but that's because we don't have answer testing. When I try our "manual answer testing" and compare against the result from using the old stable before these backports, I get a slight difference (only really visible when you toggle back and forth between the images). I used hg bisect to narrow it down to a single changeset, but I'm not entirely sure if the difference I'm seeing is OK.

    For reference, here are the steps I followed:

    cd $YT_HG
    hg up -r 8752de74f4e3 # the old stable tip
    python setup.py develop
    cd $TRIDENT
    hg up v0.5.1
    cd examples
    python working_script.py
    mv spec_raw.png spec_raw_good.png
    cd $YT_HG
    hg up 784bb3c # the tip from this PR
    python setup.py develop
    cd $TRIDENT/examples
    python working_script.py
    open spec_raw.png spec_raw_good.png

    When I do this, you can see a slight difference in the two raw spectra. Basically, the new spectrum is slightly more stretched into the blue on the leftward side, indicating maybe a change in redshift? You can see the images here, but it shows up better if you blink them:

    Good: spec_raw.png

    New: spec_raw.png

    Using hg bisect, I was able to narrow the change down to this commit: 8846ebeea82d (https://bitbucket.org/yt_analysis/yt/commits/8846ebeea82d5f784d34cdc81534fef6c205a1aa?at=yt) from Britton's LightRay PR (https://bitbucket.org/yt_analysis/yt/pull-requests/2345). But I'm not sure about it. I guess the target distance by default in the old version wasn't in comoving coordinates? But the working script trident is using is using a dataset with a really low redshift (i.e. z = 0.007), so presumably this would be a really small effect, which is I guess what we're seeing.

    @brittonsmith What do you think? Does this make sense? Do we in fact want to be casting things to comoving coords and we were just doing it incorrectly in the old version?

    For reference, the "working_script" we're using in Trident is as follows:

    # Example of a currently working script using trident to generate a COS
    # spectrum from a non-cosmological dataset
    import os
    import trident as tri
    # Define the dataset and the coordinates of the start/end of the ray
    fn = 'enzo_cosmology_plus/RD0009/RD0009'
    ray_start = [0,0,0]
    ray_end = [1,1,1]
    # Make a LightRay object for the temperature, density, and metallicity
    # fields of our dataset at redshift_start = redshift_end = 0.0.  Include
    # HI field calculated in simulation to override the Cloudy estimation of HI.
    # Save LightRay to ray.h5 and use it locally as ray object.
    ray = tri.make_simple_ray(fn, start_position=ray_start,
                              end_position=ray_end, data_filename='ray.h5',
                              fields=['density', 'temperature', 'metallicity',
    # Now use the ray object to actually generate an absorption spectrum
    # Use the settings (spectral range, LSF, and spectral resolution) for COS
    # And save it as an output hdf5 file and plot it to an image.
    sg = tri.SpectrumGenerator('COS')
    # "Final" spectrum with added quasar, MW background, and gaussian noise
    # (SNR=30)
    1. Britton Smith

      @chummels, yes, I believe the change is correct and the previous behavior was wrong. That changeset, which added the following line:

      target_distance = self.cosmology.quan(target_distance.to("Mpccm / h"))

      was fixing the bug reported in Issue #1258, wherein the distance calculation was using the wrong unit system. The meat of the fix is not conversion to "Mpccm/h", but using self.cosmology.quan to override the unit system to use the cosmology calculator's unit system.

      You can see this by changing the above line in the yt tip to the following:

      target_distance = target_distance.to("Mpccm / h")

      and you will get the old behavior.

  3. Nathan Goldbaum author

    Ok, I'm going to backport a few more things, merge this, then make a source distribution.