search

desihub / redrock

Redshift fitting for spectroperfectionism
BSD 3-Clause "New" or "Revised" License
22 stars 14 forks source link

add --negflux-nsig option to mask significantly negative flux #282

Closed sbailey closed 7 months ago

sbailey commented 7 months ago

@moustakas this PR adds an rrdesi --negflux-nsig option (default 5) to mask significantly negative data.

There is still some ongoing discussion about the case of sky mis-subtraction positive-negative fluctuations — masking only the negative here risks leaving positive peaks behind to fit false emission lines. So lets not merge this yet, but this provides a branch to test and a place for comments.

Default rrdesi/rrdesi_mpi usage is unchanged, and it will apply a mask on 5-sigma negative flux pixels. That threshold can be changed with --negflux-nsig (specify a positive number of sigma, the threshold will be applied to negative flux). Having written that, I could probably make this safer by adding an abs or something to prevent sign misunderstandings leading to masking positive data...

coveralls commented 7 months ago

Coverage Status

coverage: 38.471% (-0.1%) from 38.578% when pulling 2964398617bf3fbcddb60ce2b5aa468a72c48a7b on negmask into 4c665671987ce25a981ff41298f48e22ed29def5 on main.

sbailey commented 7 months ago

@moustakas you've been the most extensive user of this branch. Have you seen any evidence that masking significantly negative flux leads to a bias of false positive fits to unmasked positive flux, e.g. from sky mis-subtraction ringing? Or have your tests been focused on other things and you couldn't say either way?

FYI, @rongpu is investigating improvements upstream, but we could still proceed with this PR as a smaller step anyway.

moustakas commented 7 months ago

@sbailey, testing this branch has been pretty low priority, although I did run with it on about 70,000 objects as part of my most recent template testing and there were no major problems that I noticed. A more careful test would be to loop through spectra and just count how many pixels would be masked, but I don't have any cycles to do this.

Anecdotally, this branch fixes this issue (and we get the correct redshift now!), so I'm happy to merge--- https://github.com/desihub/redrock/issues/281

For your consideration, I also think that this change should be the default (but perhaps in a separate PR, tied to Jura).

sbailey commented 7 months ago

Merging now. FYI, this change is the default at 5 sigma, i.e. the option can be used to change to to something other than 5 sigma, but if you don't specify anything you get a 5 sigma negative flux mask.