desihub / gpu_specter

Scratch work for porting spectroperfectionism extractions to GPUs
BSD 3-Clause "New" or "Revised" License
2 stars 3 forks source link

Add timing to spex and refactor most to submodule #37

Closed dmargala closed 4 years ago

dmargala commented 4 years ago

This PR is getting a little out of hand so I want to pause and get some feedback.

I started this by implementing some timing in spex. Then I moved pieces of spex to a "core" submodule. Now most of what was in spex is in gpu_specter.core. I'm getting a little stuck finding the right level of modularity to enable more experimentation with parallelism strategies (gpu, async i/o, etc). I think this is at an okay place until I have a more concrete strategy to target.

dmargala commented 4 years ago

I added docstrings to all the new classes and functions. Thanks for catching those.

I performed the following comparison of spex and extract_frame, with and without mpi:

# set common args
args="--nspec 50 -w 5760.0,7620.0,0.8 -i py/gpu_specter/test/data/preproc-r0-00051060.fits -p py/gpu_specter/test/data/psf-r0-00051060.fits"

# refactor-spex with and without mpi
git checkout refactor-spex
mpiexec -n 4 python bin/spex --mpi -o refactor-spex-with-mpi.fits $args
python bin/spex -o refactor-spex-without-mpi.fits $args

# spex master with and without mpi
git checkout master
mpiexec -n 4 python bin/spex --mpi -o master-with-mpi.fits $args
python bin/spex -o master-without-mpi.fits $args
# strict regression test
fitsdiff master-with-mpi.fits refactor-spex-with-mpi.fits
fitsdiff master-without-mpi.fits refactor-spex-without-mpi.fits

Both of those show "no differences found". However, the following comparisons show ~18% of pixels are different in the flux HDU.

fitsdiff master-without-mpi.fits master-with-mpi.fits
fitsdiff refactor-spex-without-mpi.fits refactor-spex-with-mpi.fits

Hopping over to the branch with the new compare-frame script to confirm that the differences are pretty small:

git checkout add-test-script
python bin/compare-frame -a master-with-mpi.fits -b master-without-mpi.fits

(f_a, f_b):
   isclose:  99.96%
(f_a - f_b)/sqrt(var_a + var_b):
     1e-05:  99.95%
    0.0001: 100.00%
     0.001: 100.00%
      0.01: 100.00%
       0.1: 100.00%
       1.0: 100.00%

python bin/compare-frame -a refactor-spex-with-mpi.fits -b refactor-spex-without-mpi.fits

(f_a, f_b):
  isclose:  99.96%
(f_a - f_b)/sqrt(var_a + var_b):
    1e-05:  99.95%
   0.0001: 100.00%
    0.001: 100.00%
     0.01: 100.00%
      0.1: 100.00%
      1.0: 100.00%
# cleanup
rm refactor-spex-with-mpi.fits
rm refactor-spex-without-mpi.fits
rm master-with-mpi.fits
rm master-without-mpi.fits

So it looks like there are some differences between mpi and without mpi but it seems like a precision issue and is already present in master.

sbailey commented 4 years ago

The with/without MPI differences are concerning in principle, but as you note they are present in master too so ok to merge this refactor as-is.