Improvement to spatial flexure #1870

debora-pe · 2024-11-13T22:46:59Z

This work makes improvements to the spatial flexure calculation and add a QA plot to check the flexure.
Documentation was also added.

It addresses issue #1843

Dev-Suite PR pypeit/PypeIt-development-suite#339

rcooke-ast

Thanks @debora-pe - Overall this looks good, but it will be good to make sure the dev suite passes. Also, it would be good to check what happens when the slit edges are quite diagonal. I also have a few general questions:

(1) Could you mention the reason why this new algorithm is required?
(2) My main concern is that the spectral smashing might smear out the slit edges. Is there are reason that the ross-correlation needs to be done in 1D instead of 2D (along one dimension)?
(3) I quite like that the cross-correlation is done on the sobel images with this algorithm. Would it also work to use the old algorithm, but instead use the sobel edges? This would avoid the smashing.

rcooke-ast · 2024-11-19T10:10:32Z

pypeit/core/flexure.py

+    sci_sobel, _ = trace.detect_slit_edges(_sciimg, bpm=bpm)
+    slits_sobel, _ = trace.detect_slit_edges(slitmask, bpm=bpm)
+    # collapse both sobel images along the spectral direction
+    sci_smash, _, _ = sigma_clipped_stats(sci_sobel, axis=0, mask=bpm)


Does this work well in cases where the slit edges are significantly tilted wrt detector columns?

Yes, it does seems to work well. I was worried too at first, but if the slits edges are tilted for the trace image they will be tilted also for the science image.

rcooke-ast · 2024-11-19T10:11:38Z

pypeit/core/flexure.py

+    sci_smash, _, _ = sigma_clipped_stats(sci_sobel, axis=0, mask=bpm)
+    slits_smash, _, _ = sigma_clipped_stats(slits_sobel, axis=0, mask=bpm)
+    # remove nan values
+    sci_smash[np.isnan(sci_smash)] = 0


Is it possible to remove the nan values from the 2D image, instead (before smashing)?

The nan values are actually created by sigma_clipped_stats when encounters the masked values. This is why I added it here.

Right, OK... Since the default is cenfunc=median, could you instead call sigma_clipped_stats with cenfunc=np.nanmedian, as this will ignore the masked pixels instead of making them nans. Could this work?

I'm also very surprised that a median operation is still able to correctly determine the flexure when the slits are tilted. Intuitively, this doesn't make sense to me, especially given that a median operation will reject the significant edges that are enhanced by the sobel filter. Do you have any example QA plots to share showing the two 1D inputs (sci_smash and slits_smash), as well as the peak of the cross-correlation for your example (LRIS) as well as a tilted spectrograph (either ESI or HIRES)?

Perhaps one possible solution is to consider a row-by-row cross-correlation of the Sobel images (possibly with a median filter applied), and then do the cross-correlation along one axis. Something like this:

corr = signal.fftconvolve(sci, np.fliplr(slits), mode='same', axes=0)

(although, you'd need to check if it's a fliplr or flipud, and if the axes variable is correct... you'd want them both along the spatial direction).

rcooke-ast · 2024-11-19T10:12:55Z

pypeit/core/flexure.py

+    sci_smash[np.isnan(sci_smash)] = 0
+    slits_smash[np.isnan(slits_smash)] = 0
+    # invert the negative values
+    sci_smash[sci_smash < 0] *= -1


Is this needed? I think keeping the left edges and right edges as +ve/-ve might actually help the cross-correlation.

I tested this a lot and it seems that inverting the negative value is still better than keeping them negative. In many cases it does not make much difference, but in some cases the cross-correlation is better with the inversion.

I wonder if the median operation is "cancelling" the positive and negative values for slits that are close together? This could explain why making both left and right edges positive helps determine the slit location.

rcooke-ast · 2024-11-19T10:16:56Z

pypeit/core/flexure.py

+
+    """
+    debug = True if outfile is None else False
+    # TODO: should we use initial or tweaked slits in this plot?


I think it's the tweaked slits that you're after. As you noted above on line 85, whatever is used here needs to be consistent with that choice. I think once the tweaked slits are available, they get used.

yes, I believe that is correct.

Well in the code above I'm still using the initial slits (not the tweaked ones) since:
slitmask = slits.slit_img(initial=True, exclude_flag=slits.bitmask.exclude_for_flexure)
It seems to me that the initial ones should work better in the cross-correlation, and therefore this plot should be consistent with that. But I do not want users to get confused when seeing this plot and comparing with what is actually use for reduction, i.e., tweaked slits if tweak_slits=True

pypeit/core/flexure.py

profxj

a few minor comments from me.

am really happy to see this code advanced.

doc/figures/r230417_01033_DET01_spat_flex_corr.png

pypeit/core/arc.py

profxj · 2024-11-20T03:12:02Z

pypeit/core/flexure.py

+    sci_smash[np.isnan(sci_smash)] = 0
+    slits_smash[np.isnan(slits_smash)] = 0
+    # invert the negative values
+    sci_smash[sci_smash < 0] *= -1


profxj · 2024-11-20T03:14:05Z

pypeit/core/flexure.py

-    #tslits_shift = trace_slits.shift_slits(tslits_dict, lag_max)
-    # Now translate the tilts
+        # 2D plot
+        spat_flexure_qa(sciimg, slits, shift, gpm=np.logical_not(bpm), vrange=qa_vrange)


remove this line? it is duplicated below.
or is the idea to generate 2 PNGs?

No, this one is used only in debug mode and plot in a matplot window the whole image. No PNG is created. I uses the same script, but it does slightly different things.

profxj · 2024-11-20T03:14:50Z

pypeit/core/flexure.py

+
+    """
+    debug = True if outfile is None else False
+    # TODO: should we use initial or tweaked slits in this plot?


yes, I believe that is correct.

profxj · 2024-11-20T03:19:34Z

pypeit/images/combineimage.py

+        if np.sum(no_nan) > 0:
+            if np.any(np.absolute(np.diff(spat_flex[no_nan])) > 0.1):
+                msgs.warn(f'Spatial flexure is not consistent for all images being combined: {spat_flex}.')
+                comb_spat_flex = np.round(np.mean(spat_flex[no_nan]),3)


is there a preference to rounding to the nearest pixel instead of staying a float ?

It is only rounding to the 3rd decimal. It's still a float. But the mean calculation can make it to be a very long number.

pypeit/images/rawimage.py

debora-pe

@rcooke-ast @profxj I have addressed your comments.
I have extensively tested these changes with all the Keck LRIS data in the Dev Suite and all looks good. I will anyway re-run the whole Dev Suite.

@rcooke-ast Here are my answers to your questions:

(1) Could you mention the reason why this new algorithm is required?

The old algorithm was not working well. A peak in the cross-correlation was often not found and the spatial flexure was not corrected.

(2) My main concern is that the spectral smashing might smear out the slit edges. Is there are reason that the ross-correlation needs to be done in 1D instead of 2D (along one dimension)?

As I mentioned in another comment, I was little worried too, but after a lot of testing (also with more tilted slit edges) the correction looks reasonable. I thought about a 2D cross-correlation but I wasn't able to find a way to do it without adding a lot of runtime (it's probably me that I don't know how to deal with it)

(3) I quite like that the cross-correlation is done on the sobel images with this algorithm. Would it also work to use the old algorithm, but instead use the sobel edges? This would avoid the smashing.

I have also tested a lot using the sobel edges, and the correction is not consistently good.

debora-pe · 2024-11-23T00:01:27Z

pypeit/core/flexure.py

+    sci_sobel, _ = trace.detect_slit_edges(_sciimg, bpm=bpm)
+    slits_sobel, _ = trace.detect_slit_edges(slitmask, bpm=bpm)
+    # collapse both sobel images along the spectral direction
+    sci_smash, _, _ = sigma_clipped_stats(sci_sobel, axis=0, mask=bpm)


Yes, it does seems to work well. I was worried too at first, but if the slits edges are tilted for the trace image they will be tilted also for the science image.

debora-pe · 2024-11-23T00:03:12Z

pypeit/core/flexure.py

+    sci_smash[np.isnan(sci_smash)] = 0
+    slits_smash[np.isnan(slits_smash)] = 0
+    # invert the negative values
+    sci_smash[sci_smash < 0] *= -1


I tested this a lot and it seems that inverting the negative value is still better than keeping them negative. In many cases it does not make much difference, but in some cases the cross-correlation is better with the inversion.

debora-pe · 2024-11-23T00:05:13Z

pypeit/core/flexure.py

-    #tslits_shift = trace_slits.shift_slits(tslits_dict, lag_max)
-    # Now translate the tilts
+        # 2D plot
+        spat_flexure_qa(sciimg, slits, shift, gpm=np.logical_not(bpm), vrange=qa_vrange)


No, this one is used only in debug mode and plot in a matplot window the whole image. No PNG is created. I uses the same script, but it does slightly different things.

pypeit/core/flexure.py

debora-pe · 2024-11-23T00:13:13Z

pypeit/core/flexure.py

+
+    """
+    debug = True if outfile is None else False
+    # TODO: should we use initial or tweaked slits in this plot?


Well in the code above I'm still using the initial slits (not the tweaked ones) since:
slitmask = slits.slit_img(initial=True, exclude_flag=slits.bitmask.exclude_for_flexure)
It seems to me that the initial ones should work better in the cross-correlation, and therefore this plot should be consistent with that. But I do not want users to get confused when seeing this plot and comparing with what is actually use for reduction, i.e., tweaked slits if tweak_slits=True

debora-pe · 2024-11-23T00:15:18Z

pypeit/images/combineimage.py

+        if np.sum(no_nan) > 0:
+            if np.any(np.absolute(np.diff(spat_flex[no_nan])) > 0.1):
+                msgs.warn(f'Spatial flexure is not consistent for all images being combined: {spat_flex}.')
+                comb_spat_flex = np.round(np.mean(spat_flex[no_nan]),3)


It is only rounding to the 3rd decimal. It's still a float. But the mean calculation can make it to be a very long number.

debora-pe · 2024-11-23T00:18:54Z

pypeit/core/flexure.py

+    sci_smash, _, _ = sigma_clipped_stats(sci_sobel, axis=0, mask=bpm)
+    slits_smash, _, _ = sigma_clipped_stats(slits_sobel, axis=0, mask=bpm)
+    # remove nan values
+    sci_smash[np.isnan(sci_smash)] = 0


The nan values are actually created by sigma_clipped_stats when encounters the masked values. This is why I added it here.

rcooke-ast

Thanks for the responses @debora-pe! I think it would be helpful to see some example plots of the two signals that you are correlating, for examples with tilted and non-tilted slits.

I always thought the flexure was done by correlating the slits and science frame along the spatial direction, but this seems to never have been the case. I can see that your algorithm is an improvement over what was previously in the code, but I've also proposed another possible solution (correlating 2D images along one direction). I think this would work best, particularly if we used the sobel filtering (without converting -ve numbers to +ve numbers). What do you think?

rcooke-ast · 2024-11-26T09:41:16Z

pypeit/core/flexure.py

+    sci_smash, _, _ = sigma_clipped_stats(sci_sobel, axis=0, mask=bpm)
+    slits_smash, _, _ = sigma_clipped_stats(slits_sobel, axis=0, mask=bpm)
+    # remove nan values
+    sci_smash[np.isnan(sci_smash)] = 0


Right, OK... Since the default is cenfunc=median, could you instead call sigma_clipped_stats with cenfunc=np.nanmedian, as this will ignore the masked pixels instead of making them nans. Could this work?

rcooke-ast · 2024-11-26T09:49:14Z

pypeit/core/flexure.py

+    sci_smash, _, _ = sigma_clipped_stats(sci_sobel, axis=0, mask=bpm)
+    slits_smash, _, _ = sigma_clipped_stats(slits_sobel, axis=0, mask=bpm)
+    # remove nan values
+    sci_smash[np.isnan(sci_smash)] = 0


I'm also very surprised that a median operation is still able to correctly determine the flexure when the slits are tilted. Intuitively, this doesn't make sense to me, especially given that a median operation will reject the significant edges that are enhanced by the sobel filter. Do you have any example QA plots to share showing the two 1D inputs (sci_smash and slits_smash), as well as the peak of the cross-correlation for your example (LRIS) as well as a tilted spectrograph (either ESI or HIRES)?

rcooke-ast · 2024-11-26T12:09:48Z

pypeit/core/flexure.py

+    sci_smash[np.isnan(sci_smash)] = 0
+    slits_smash[np.isnan(slits_smash)] = 0
+    # invert the negative values
+    sci_smash[sci_smash < 0] *= -1


I wonder if the median operation is "cancelling" the positive and negative values for slits that are close together? This could explain why making both left and right edges positive helps determine the slit location.

rcooke-ast · 2024-11-26T12:20:04Z

pypeit/core/flexure.py

+    sci_smash, _, _ = sigma_clipped_stats(sci_sobel, axis=0, mask=bpm)
+    slits_smash, _, _ = sigma_clipped_stats(slits_sobel, axis=0, mask=bpm)
+    # remove nan values
+    sci_smash[np.isnan(sci_smash)] = 0


Perhaps one possible solution is to consider a row-by-row cross-correlation of the Sobel images (possibly with a median filter applied), and then do the cross-correlation along one axis. Something like this:

corr = signal.fftconvolve(sci, np.fliplr(slits), mode='same', axes=0)

(although, you'd need to check if it's a fliplr or flipud, and if the axes variable is correct... you'd want them both along the spatial direction).

debora-pe · 2024-12-05T23:17:14Z

Thanks for the responses @debora-pe! I think it would be helpful to see some example plots of the two signals that you are correlating, for examples with tilted and non-tilted slits.

I always thought the flexure was done by correlating the slits and science frame along the spatial direction, but this seems to never have been the case. I can see that your algorithm is an improvement over what was previously in the code, but I've also proposed another possible solution (correlating 2D images along one direction). I think this would work best, particularly if we used the sobel filtering (without converting -ve numbers to +ve numbers). What do you think?

Allright @rcooke-ast . Thank you for pushing back on this. I have extensively tests several methods and indeed using a row-by-row cross-correlation works better for very curved slits (like ESI). Bright objects can affect the cross-correlation a lot, so I have added a way to approximately mask the object. Also, I found that the cross-correlation works better with the edge image generated from the sobel image, so I am using that. Let me know what you think.
I have tested these changes with all the Dev-Suite data that we have for LRIS, DEIMOS, HIRES, ESI, SHANE and have added some more tests in the Dev-Suite PR.

@profxj , if you can, take a look as well, since I have made substantial changes.

I'll run the full Dev-Suite as well.

rcooke-ast

Looks good to me, thanks @debora-pe! Just two minor comments from me, and you have my 👍 to merge when the dev suite passes.

rcooke-ast · 2024-12-06T12:16:21Z

pypeit/core/flexure.py

+    # mask (as much as possible) the objects on the slits to help the cross-correlation
+    # need to copy the bpm to avoid changing the input bpm
+    _bpm = np.zeros_like(_sciimg, dtype=int) if bpm is None else copy.deepcopy(bpm)
+    for i in range(slits.left_init.shape[1]):


slits.nslits might be a little clearer

rcooke-ast · 2024-12-06T12:17:46Z

pypeit/core/flexure.py

+    # need to copy the bpm to avoid changing the input bpm
+    _bpm = np.zeros_like(_sciimg, dtype=int) if bpm is None else copy.deepcopy(bpm)
+    for i in range(slits.left_init.shape[1]):
+        left_edge = slits.left_init[:, i].astype(int)


Not a big deal, but should we do:
np.round(slits.left_init[:, i]).astype(int)
and likewise for right_edge?

…nto spatflex_improve

debora-pe · 2024-12-20T02:41:23Z

Finally I was able to run the Dev Suite (my computer was annoying!). All good. The only failure is the usual setup_gui unit test that my computer have problem with.

Test Summary
--------------------------------------------------------
--- PYTEST PYPEIT UNIT TESTS PASSED  257 passed, 63 warnings in 340.40s (0:05:40) ---
--- PYTEST UNIT TESTS FAILED  1 failed, 151 passed, 168 warnings in 2751.88s (0:45:51) ---
--- PYTEST VET TESTS PASSED  65 passed, 101 warnings in 5384.05s (1:29:44) ---
--- PYPEIT DEVELOPMENT SUITE PASSED 249/249 TESTS  ---
Testing Started at 2024-12-16T17:18:33.164290
Testing Completed at 2024-12-19T15:03:25.137859
Total Time: 2 days, 21:44:51.973569

debora-pe added 17 commits September 16, 2024 14:41

still nowhere

0e52afa

Merge branch 'develop' into spatflex_improve

aca3e44

final version?

a5c44b3

some stuff for a QA plot

b49a79a

Merge branch 'develop' into spatflex_improve

f06bbd8

finalized QA plot

0a029a8

small fix

c84e3e7

small fix

e526c1e

improved

a447a20

improved

dd06e5c

small fixes

d286930

docs

5cc3f32

docs

3245c8c

Merge branch 'develop' into spatflex_improve

617e550

Merge branch 'release' into spatflex_improve

d82cd5f

added to release doc

3f3237d

forgot one parameters

40cd1d4

debora-pe requested review from profxj and rcooke-ast November 15, 2024 23:29

rcooke-ast changed the base branch from develop to dev2rel November 16, 2024 21:05

Base automatically changed from dev2rel to develop November 18, 2024 16:28

Merge branch 'develop' into spatflex_improve

8ff7bda

rcooke-ast requested changes Nov 19, 2024

View reviewed changes

found a bug an fixed it

8cf8948

debora-pe mentioned this pull request Nov 20, 2024

Spatflex improve tests pypeit/PypeIt-development-suite#339

Merged

fix the fix

9f05c66

profxj approved these changes Nov 20, 2024

View reviewed changes

debora-pe added 3 commits November 21, 2024 17:20

small improv

da9e7a1

Merge branch 'develop' into spatflex_improve

80676b4

small fix

279975d

debora-pe commented Nov 23, 2024

View reviewed changes

rcooke-ast requested changes Nov 26, 2024

View reviewed changes

debora-pe added 3 commits December 4, 2024 16:26

followed Ryan's suggestion

55b7a72

small thing

df50e77

small thing

9469cf4

rcooke-ast approved these changes Dec 6, 2024

View reviewed changes

debora-pe and others added 6 commits December 9, 2024 17:12

last comments

5dcddec

Merge branch 'develop' into spatflex_improve

9b233e1

Merge branch 'develop' into spatflex_improve

da95545

small fix

782162d

Merge branch 'spatflex_improve' of https://github.com/pypeit/PypeIt i…

18918ee

…nto spatflex_improve

Merge branch 'develop' into spatflex_improve

3e5a48e

debora-pe merged commit eb54214 into develop Dec 20, 2024
26 checks passed

debora-pe deleted the spatflex_improve branch December 20, 2024 04:28

debora-pe mentioned this pull request Dec 20, 2024

Improve automatic spatial flexure correction #1843

Closed

Improvement to spatial flexure #1870

Improvement to spatial flexure #1870

Conversation

debora-pe commented Nov 13, 2024 • edited Loading

rcooke-ast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

profxj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

debora-pe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcooke-ast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

debora-pe commented Dec 5, 2024

rcooke-ast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

debora-pe commented Dec 20, 2024

debora-pe commented Nov 13, 2024 •

edited

Loading