Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GMOS tilted slits #1827

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

GMOS tilted slits #1827

wants to merge 5 commits into from

Conversation

jhod0
Copy link
Contributor

@jhod0 jhod0 commented Jul 4, 2024

Currently the GMOS mask design info in PypeIt does not allow for tilted slits, this PR tries to implement that. @SunilSimha had given me some quick instructions on what to change for this.

I believe this is now working for GMOS-South with data of mine taken in January with the newly-installed detectors. I have managed to do PypeIt reductions with these changes which appear, to me, to match up slits correctly with their mask design info.


One small "issue" is that the predicted and empirically measured slit edges seem to be systematically off by ~10pix. Is this a negligible offset, or something that actually needs to be corrected? In any case, it appears to work the same for tilted and non-tilted slits.

The following is from a slit mask where the slits centered at ~533 and ~652 are tilted.
Here are the measures slit edges for one mask, loaded in from a Slits....tgz calibration file:

In [9]: slittrace.left_init.mean(axis=0)
Out[9]: 
array([ 68.54173947,  98.00885328, 138.69572981, 196.51892295,
       294.17315658, 322.87968086, 350.14648271, 392.40419934,
       426.14172727, 460.50121   , 501.74282665, 568.97874474,
       600.55261234, 629.82433676, 676.31800289, 707.15505595,
       737.11302102, 775.1422126 , 816.98769996, 848.45596147,
       910.80904232, 939.13737437])

In [10]: slittrace.right_init.mean(axis=0)
Out[10]: 
array([  94.16692426,  123.49548875,  189.31895314,  247.33036532,
        319.74443138,  348.15803612,  381.62238749,  423.97014995,
        457.68178311,  498.56045817,  564.63668824,  594.41294097,
        626.10249187,  673.99849644,  701.67494821,  732.69161189,
        762.73213288,  813.04315201,  842.44770077,  889.72812725,
        936.29172014, 1002.34888583])

And here are the predicted slit edges from get_maskdef_slitedges(), which I printed out from planting a pdb.set_trace() and during a regular PypeIt reduction.

(Pdb) print(left_edges[sortindx])
[ 58.94504103  88.22990965 128.77500789 186.36529393 238.95549959
 260.01355608 268.30322747 283.89777812 312.56815682 339.34384325
 381.82503537 415.51702612 449.99433915 491.2466831  559.17875393
 590.07757776 619.1093391  665.44344915 696.69455186 726.47917565
 754.3651992  764.40233786 805.99724907 837.70418582 885.15920932
 899.93692467 928.15075437]
(Pdb) print(right_edges[sortindx])
[ 83.95808537 113.24294814 178.80108637 236.42681956 257.70445191
 266.26320833 274.55288395 308.92853715 337.5667816  370.59210421
 413.07328887 446.76528381 487.49224481 553.71021316 584.17735362
 615.07618334 662.91268769 690.44203954 721.72529776 751.47777214
 760.62288581 801.90025684 830.99583934 878.32691035 891.40885725
 924.94994212 990.6472606 ]

Copy link
Collaborator

@kbwestfall kbwestfall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jhod0 . Thanks! This looks good to me. Two requests:

  • Can you add a note on this in the doc/releases/1.16.1dev.rst doc?
  • We should check with @debora-pe about the offset you see in the predictions vs. the measurements. I expect this should be okay because the code should determine this offset. I.e., the exact prediction doesn't really matter, as long as the code is able to correctly match the slit mask design to the slits detected in the image.

@jhod0
Copy link
Contributor Author

jhod0 commented Jul 10, 2024

Thanks @kbwestfall , I added a short description under "Instrument-Specific Updates" in that file.

Should this change also be applied to the GMOS-North implementation? Right now this is only on GMOS-South. I have no GMOS-North data to test this on, but I would hope the mask design stuff is identical between the two instruments.

Nevermind - the update is in the parent class of both GMOS-South and GMOS-North, so it is already applied to both.

@debora-pe
Copy link
Collaborator

Hi @jhod0 . Thanks! This looks good to me. Two requests:

  • Can you add a note on this in the doc/releases/1.16.1dev.rst doc?
  • We should check with @debora-pe about the offset you see in the predictions vs. the measurements. I expect this should be okay because the code should determine this offset. I.e., the exact prediction doesn't really matter, as long as the code is able to correctly match the slit mask design to the slits detected in the image.

Hi @jhod0 Thank you for doing this.
@kbwestfall is right in that the offset is not a problem as long as the cross correlation between measured and predicted is correct.
We should still test these changes with other Gemini-N/S datasets. We have several in our Dev Suite. If that is not possible for you @jhod0, we can do it (I may be able to run the test sometime next week).

@jhod0
Copy link
Contributor Author

jhod0 commented Jul 11, 2024

Great! I certainly can test on other GMOS data sets, but it will be on the back burner for me and I might not get to it for a few weeks.

@@ -21,6 +21,11 @@ Functionality/Performance Improvements and Additions
Instrument-specific Updates
---------------------------

- GMOS-South: PypeIt reductions of GMOS-South MOS observations now properly
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is not only for GMOS-S but also GMOS-N

@debora-pe
Copy link
Collaborator

I run the tests. All good (the few failures are not related)

Test Summary
--------------------------------------------------------
--- PYTEST PYPEIT UNIT TESTS FAILED  3 failed, 253 passed, 66 warnings in 339.50s (0:05:39) ---
--- PYTEST UNIT TESTS FAILED  2 failed, 146 passed, 166 warnings in 1625.44s (0:27:05) ---
--- PYTEST VET TESTS PASSED  61 passed, 97 warnings in 5141.07s (1:25:41) ---
--- PYPEIT DEVELOPMENT SUITE PASSED 239/239 TESTS  ---
Testing Started at 2024-07-18T15:58:02.563299
Testing Completed at 2024-07-19T04:58:47.612689
Total Time: 13:00:45.049390

Unfortunately, we have only one dataset in the Dev Suite that uses the mask definition information and it's GMOS-S with non-tilted slits. So, I could not check much, but the results for the non-tilted slits data did not change, so I guess it's good thing :)
So, I think this can be merged.

@profxj
Copy link
Collaborator

profxj commented Nov 21, 2024

@jhod0 -- need any help finishing this PR off?
Just checking.

@kbwestfall
Copy link
Collaborator

@jhod0 -- need any help finishing this PR off? Just checking.

I think we're waiting for relevant test data to become public, which I think is sometime in Jan, if I remember right.

@jhod0
Copy link
Contributor Author

jhod0 commented Nov 25, 2024

Yes Kyle is right. The data were all taken on January 17th, I believe it's a one year proprietary period and they will be public this coming January 17th? Couldn't quickly verify the proprietary period.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants