specutils icon indicating copy to clipboard operation
specutils copied to clipboard

Some updates to specutils.analysis functions

Open chloe-mt-cheng opened this issue 3 years ago • 1 comments

  • Updated _normalize_for_template_matching(): changed np.sum to np.nansum on lines 60 and 61. Due to resampling of the spectra (resampling methods fill arrays with NaNs to have points beyond the edges set to NaN), all redshifts compared in template_redshift() were producing a chi2 of NaN (even for a test case using the correct redshift).
  • Updated _chi_square_for_templates(): changed np.sum to np.nansum on line 137 for the same reason as above.
  • Updated template_redshift(): Added final_spectrum = redshifted_spectrum on line 292 and added final_spectrum to return instead of redshifted spectrum. This is important specifically for if you feed a list of redshifts - the function was not returning the spectrum redshifted by the best-fitting redshift (i.e. the one corresponding to chi2_min), but rather was returning the spectrum redshifted by the last redshift in the list (not necessarily the best-fitting one).

chloe-mt-cheng avatar Mar 22 '22 21:03 chloe-mt-cheng

Thanks @chloe-mt-cheng! I'll take a look at updating the failing tests, but these seem like sensible changes.

rosteen avatar Mar 28 '22 16:03 rosteen

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (08e77ce) 70.78% compared to head (34ad35d) 70.80%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #937      +/-   ##
==========================================
+ Coverage   70.78%   70.80%   +0.02%     
==========================================
  Files          61       61              
  Lines        4213     4216       +3     
==========================================
+ Hits         2982     2985       +3     
  Misses       1231     1231              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Jan 30 '24 22:01 codecov[bot]

@chloe-mt-cheng Sorry that I dropped the ball on this for so long! I updated the tests after confirming that the differences make sense, and made one more change to ensure that the same number of bins get summed for the numerator and denominator in the nansums.

rosteen avatar Jan 30 '24 22:01 rosteen