Trace fix improvements #2386
Trace fix improvements #2386
Conversation
Also use different psfs to convolve the spectrum
therefore the filtering size needs to depend on that
py/desispec/trace_shifts.py
Outdated
| ivar=ivar[fiber,ok], | ||
| hw=3., calibrate=True) | ||
| if fiber %10==0 : | ||
| log.info("Wavelength offset %f +/- %f for fiber #%03d at wave %f "%(dwave, err, fiber, block_wave)) |
There was a problem hiding this comment.
Minor style comment, but for the record so that it doesn't propagate to more code:
When formatting strings for logging, it is better to use the structure
log.info("Wavelength offset %f +/- %f for fiber #%03d at wave %f", dwave, err, fiber, block_wave)so that the string formatting is only evaluated by the logger if the log level would actually print it. In practice we almost always have INFO-level on so this particular case doesn't make a performance difference, but the readability is nearly the same as old-syle %-formatting and we have had cases in the past of string evaluation of unprinted debug-level logging taking a significant amount of time, so this style is something to keep in mind when adding new log messages.
Personal opinion: I'm also fine with pre-evaluated new-style format strings for INFO-level and above if the author feels it helps with readability, e.g.
log.info(f"Wavelength offset {dwave} +/- {err} for fiber #{fiber:03d} at wave {block_wave}")My basic motivation is that readability trumps performance given that we use INFO-level for production running anyway so there isn't a performance impact. This should not be used for DEBUG-level logging though, due to performance issues especially if it is deep in loops.
[actual review of algorithmic changes takes more thought/time...]
|
I think to avoid feature creep and very large patch, I have decided to test what we have here. To my surprise this already lead to significant improvements. I looked at the xmatch of velocities ( from redrock-) to APOGEE.
And here, the MAD wrt to APOGEE changes from 1.3 km/s to 1.1 km/s. So I think that's good evidence of things improving and I think it would be good to commit this.
One thing that I feel makes these changes harder to analyse is that these internal/external wavelength offsets for each fiber and wavelength bins are not saved anywhere (other than printed in the log and saved on average in MEANDY). I think for future improvements, it'd be beneficial to save these kinds of offsets (i.e. either in psf- file or some other kind of output file). It'd be ~ 2500 numbers per single frame, so it's not that much. But maybe that should be dealt with separately. |
|
I've verified on the the test subset of data processed /global/cfs/cdirs/desi/spectro/redux/koposov/wavelength_test_bulk_new/ |
get rid of useless warning
There was a problem hiding this comment.
-
I agree weighting with ivar x flux^2 x (flux>0) is better than ivar x flux x (flux>0) to get the effective wavelength in a bin.
-
Subtracting the continuum is a good idea to improve the precision on the cross-match (similarily to what was done to the match of the sky)
-
I see the effort to sample the PSF across wavelength and fibers to convolve the external reference spectrum (original code was using only wavelength and fiber for the PSF)
-
I ran the code on a random dark time exposure and 3 cameras and found tiny shifts <0.01A as expected
-
I have only one change request: you wrote a comment "The reason why we oversample is unclear to me".
It is there because the wavelength arrays with the boxcar extraction are not aligned and for this reason I thought oversampling before stacking would help. Given the careful study you have done, maybe you have seen that this does not help, in which case please change the code, otherwise, you may remove the comment.
I think we can merge after that. Thank you for all the work.
fix a few typos in comments
|
Thanks for looking over the patch @julienguy . On the resampling, My comment was just a thought (it wasn't obvious to me there would be benefit from oversampling), but I didn't verify that. So I've removed that comment. desispec/py/desispec/trace_shifts.py Line 725 in fe88bbd (it's now line 688 in my patch). I did not fully follow the reason behind the y[-1]-y[0] in this original code so my code is just a more straightforward 2d grid in x,y, but maybe I missed the reason for that. |
…into trace_fix_improvements
Hi,
This is a PR WIP trying to address #2380 (and maybe also improve systematic rv floor)
ATM I don't think it fixes anything much but currently does a couple of things I was hoping could help.
I did also try to restructure the code a little bit/reduce duplication to make it easier to change.
I would ideally like to merge this type of patch even if it cannot fully fix #2380 (when it is ready and assuming nothing gets worse)