bkg_spectrum to take func instead of string

[pllim]

This is a direct follow-up of unreleased feature added in #253 so behavior change can be done without deprecation or change log.

I am just wondering what it would have looked like.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.11%. Comparing base (6a0b915) to head (f055c22).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #255      +/-   ##
==========================================
- Coverage   87.19%   87.11%   -0.08%     
==========================================
  Files          13       13              
  Lines        1179     1172       -7     
==========================================
- Hits         1028     1021       -7     
  Misses        151      151              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[kecnry]

If we do this, I think we'd want to mirror that in the class-level statistic parameter, but that has additional logic and restriction to just median/average, which might be difficult to make fully general 🤷‍♂️

[pllim]

Where is the statistic? I was looking at Spectrum1D.collapse upstream which takes function.

[kecnry]

In Background (see https://github.com/astropy/specreduce/blob/main/specreduce/background.py#L41, for example)

[pllim]

Hmm... Should #253 have used this statistic instead of introducing a new keyword?

[tepickering]

there is a complaint about a conflict that needs resolving before we can merge. i would suggest making np.nanmedian the default vs np.nansum. we want a per-pixel background in most cases and the median statistic is more robust against outliers that can skew the mean. personally, i would use scipy.stats.mode in many cases and i appreciate how this change would allow me to do so.

[pllim]

@tepickering , I think I have addressed your comment. But now though, this is a breaking change. Not sure if it belongs in bugfix anymore... 😬

[hpparvi]

Hi, I'm afraid there might be some confusion regarding the logic and intended functionality of the bgk_spectrum method (sorry, I should've caught this already with #253).

• Background estimation occurs in __post_init__ using the statistic parameter, which currently accepts either average or median. The resulting 1D background estimate is stored in Background._bkg_array.

• The Background.bkg_image method creates a 2D background image by tiling the 1D array (Background._bkg_array) along the cross-dispersion axis. This results in a 2D Spectrum1D object whose pixel values vary along the dispersion axis but remain constant along the cross-dispersion axis.

• The Background.bkg_spectrum method collapses the 2D background image from Background.bkg_image back into a 1D Spectrum1D by applying a statistical function along the cross-dispersion axis. However, since all pixels along the cross-dispersion axis have identical values, choosing different statistical functions here has no practical effect.

If we want to change the statistic used for background estimation, we need to do it in Background.__post_init__, as @pllim pointed out earlier. We could change statistic to accept either a string or a custom function that accepts masked arrays without affecting existing functionality.

@kecnry, you introduced bkg_spectrum in c53ea53. Can you remind us of the original use-case for the method?

[kecnry]

Yes, I definitely think with the past few PRs all being somewhat related that we need to make sure we have a plan for what this all means and how it fits together. I'll do some digging with @gibsongreen @cshanahan1 and some old notes and will get back to you then.