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

Implement pixel-based serial CTE correction in CALACS #577

Open
jryon opened this issue Dec 16, 2022 · 74 comments
Open

Implement pixel-based serial CTE correction in CALACS #577

jryon opened this issue Dec 16, 2022 · 74 comments
Assignees

Comments

@jryon
Copy link

jryon commented Dec 16, 2022

We'd like to include a pixel-based correction for serial CTE (X-direction) in CALACS. We have not yet worked out all of the details, so much of this subject to change.

Our initial idea for the algorithm is to take each chip (WFC1, WFC2) from the input science images, split by amp (quadrants), and rotate them such that the serial CTE trails point in the +Y direction. The ACS team would update the PCTETAB to contain the serial CTE trail and trap density parameters in one or more additional extensions. Then, CALACS would run the existing CTE correction algorithm on the rotated quadrants using the serial extensions of the PCTETAB.

The serial CTE correction step would precede the current parallel CTE (Y-direction) correction. The output arrays from serial CTE correction would be input into the parallel CTE correction step, which would proceed as it currently does, using the same extensions/parameters from the PCTETAB.

@yotam-stsci Anything to add?

@jryon
Copy link
Author

jryon commented Jun 14, 2023

I'm confirming that our original vision of the algorithm for serial CTE correction in the above comment still holds.

I have confirmed that ACSCTE will work as-is on an image with the amps rotated. I took a full-frame image, rotated each amp to point the serial CTE trails towards the chip gap (+Y for WFC2 and -Y for WFC1, as if they were parallel trails) and concatenated the amps back into full 2048x4096 arrays. I created a PCTETAB with extensions 1 through 4 replaced with parameters I determined for serial CTE, and relevant header keywords updated for serial CTE. I'm currently analyzing the resulting CTE-corrected images to determine how to adjust the parameters for a better correction. I can use this technique to test and update the serial CTE model parameters before the serial CTE correction step is implemented in CALACS.

It is still true that the new PCTETAB for both parallel and serial correction will have additional extensions. It would have 9 total: 0 for the primary header, 1-4 for parallel CTE parameters, and 5-9 for serial CTE parameters. The format, shape, names, etc of ext 5-9 would be identical to that of ext 1-4, but the values in the data tables and arrays will be different. There will be additional header keywords in the primary header to specify the number of iterations for the forward model (to be determined), number of iterations of correction (PCTENSER), MJD for determining the strength of the correction (SCTEDAT1), and maybe more for serial CTE specifically.

I believe the above tests show that the underlying forward model and correction in ACSCTE don't need to be changed for serial CTE. There will need to be a new step to rotate the input arrays correctly first - clockwise 90 deg for amps A and D, counterclockwise 90 deg for amps B and C. Then, the serial CTE correction step will be run using the same code as for parallel CTE, but with the serial header keywords and parameters in the PCTETAB. Then the parallel CTE correction step will be run on the output arrays from the serial correction, rotated back to their normal orientation.

One question - does the current CTE correction algorithm use the PCTEFRAC keyword in the PCTETAB header? I'm not sure what it is for at the moment.

If you have any questions, let me know!

@mdlpstsci
Copy link
Contributor

@jryon The PCTEFRAC in the PCTETAB header is not even read by the software. The Gen 1 CTE correction does not look for a series of PCTExxxx keywords in the PCTETAB at all. The Gen 2 CTE correction does not look for this particular keyword, though it does look for others (e.g., PCTENSMD).

@jryon
Copy link
Author

jryon commented Jun 28, 2023

@mdlpstsci Great, thanks. Can you provide a list of keywords that the Gen 2 correction cares about, and if you can tell, what they are used for? I want to make sure my understanding of them lines up with what ACSCTE is doing. Thanks!

@mdlpstsci
Copy link
Contributor

mdlpstsci commented Sep 5, 2023

@mdlpstsci Remember Issue #166.

This issue has been closed as I have added a message to mitigate confusion by any user seeing the so-called error messages which are generated at the lower level.

@mdlpstsci
Copy link
Contributor

@jryon I have a few questions at this time (certainly more later).

  1. Is the serial CTE correction only to be done under umbrella of the Generation 2 CTE algorithm? I have assumed yes, but I thought it would be best to ask.

  2. Should the serial CTE correction be a separate step from the parallel CTE with separate control? If this were the case in a fully implemented sense, there would need to be a new CORR keyword. In addition, there would be new/more keyword rules, and CALACS would have to know what to do if the, for example, PCTECORR and xCTECORR, keywords were not set to the same value. I have assumed they would be two actions done for the same step => PCTECORR. This means that there is not separate control for the parallel and serial actions.

  3. Because the serial CTE is an enhancement, should this be considered Generation 3 CTE algorithm? I am thinking about CTE algorithm versions and versions of the calibration reference files. I have to check the current code to see the consistency checking done if a user chooses Algorithm 1 or 2 and a calibration reference file which is a mismatch.

@jryon
Copy link
Author

jryon commented Sep 12, 2023

@mdlpstsci Here are our answers.

  1. Yes, if by "under the umbrella" you mean using all the same enhancements that the generation 2 CTE algorithm provided. I know there are different header keywords in the PCTETAB, multiprocessing, and a few extra steps (readnoise mitigation, readout CR fixing), among other things, that exist in gen 2 but not in gen 1. We'd want to keep those gen 2 enhancements.

  2. No, I believe we want one control for both serial and parallel CTE corrections.

  3. We don't think anyone uses the generation 1 CTE algorithm, since it would require a PCTETAB that we currently do not have available to users (as far as I'm aware). I know Jamie did a lot of work to make both generations available, but I'd suggest we do not need to continue this. Perhaps we could call this generation 3, but actually just modify generation 2. In other words, I don't think we need to have generations 1, 2, and 3 available to users via CALACS at the same time. Does that make sense?

Norman's two cents on the last point: "I'm not fussy about the nomenclature, but I don't have a problem with obsoleting both the 'Gen 1' and the current 'Gen 2' versions in favor of the X+Y de-trailing.

If the new thing is still called 'Gen 2' because it is essentially two successive Gen2 corrections in orthogonal directions, I'm OK with that."

@mdlpstsci
Copy link
Contributor

mdlpstsci commented Sep 19, 2023

@jryon

  1. The primary header description of the new preliminary
    PCTETAB file says the following about the SCLBYCOL table:

    Columns 2-6 contain the real*4 CTE scaling appropriate

    It should be Columns 2-5. DONE

  2. Since the EXTNAMEs are the same for the parallel and serial FITS
    extensions, you need to set the EXTVER for the serial data to "2". DONE

  3. In my test version of your PCTETAB file, I made the
    CTE_NAME = "Par/Serial PixelCTE 2023". This was not my first
    choice, but it turns out the string has a limit in the CALACS
    code to 24 characters. Changing the length and verifying that
    there is no bad side affect turns out to be more complex
    than necessary. The CTE_VER is just there for completeness
    to what was done previously.

    CTE_NAME = "Par/Serial PixelCTE 2023"
    CTE_VER = 3.0

    I note the "Version 2" CTE had the following in the PCTETAB:
    CTE_NAME= 'PixelCTE 2017'
    CTE_VER = 2.0

    Let me know what value of CTE_NAME you would like though please limit
    the string to 24 characters - SORRY. DONE

@jryon
Copy link
Author

jryon commented Sep 19, 2023

@mdlpstsci Forgive me, I can't remember which PCTETAB file I sent you - can you remind me? It's been in flux lately so I'd like to make sure you're looking at the current one.

  1. I will fix this, thanks.
  2. Given some recent analysis, the PCTETAB may end up containing 4 extensions per amplifier for the serial correction. I'm not 100% sure that will be the case, but it's looking strongly that way. In this case, should EXTVER be 2 for all of the serial extensions, or 2 for ext 5-8, 3 for ext 9-12, 4 for ext 13-16, and 5 for ext 17-20?
  3. I am fine with the CTE_NAME = "Par/Serial PixelCTE 2023". Not many folks will be looking at this, and we might as well follow what was done before.
    Thanks!

@mdlpstsci
Copy link
Contributor

@jryon
You gave me prelim_pctetab_cte.fits. I can crack it open if need be. There are only 8 extensions this file.

As for 2. above -- No problem. You just need to change the EXTVER when the EXTNAMEs are identical to other extensions. There needs to be a programmatic way to distinguish between the EXTNAMEs. Indeed, the above scheme you note in Item 2 above would work fine. I do suggest there be a keyword in the each extension (for example, 9-12) which indicates the associated amp.

@mdlpstsci
Copy link
Contributor

@jryon
Do you have a dataset that I can use for testing a preliminary version of the updated code? Also, do you have any output where ONLY the serial CTE correction was applied, as well as the output where both serial and parallel CTE corrections were applied so I can use this for comparison.

@mdlpstsci
Copy link
Contributor

@jryon
While I am fairly sure no one will be mucking with the calibration reference files, I do not like just depending upon the position (extension number) in the file. Can you please add a new keyword in each extension (e.g., CORRTYPE="serial" or "parallel") so that I can check it to make sure that extension 1 really pertains to the "parallel" CTE (as an example). Thanks

@mdlpstsci mdlpstsci self-assigned this Oct 9, 2023
@mdlpstsci
Copy link
Contributor

@jryon I have a working version of the serial and parallel CTE correction coded in CALACS. I have used jdg301bwq for testing which is the image used in ISR ACS 2020-07. I have been trying to verify if I see any improvement. I have tested the flow of the code and I get the same results comparing Items 1, 2, and 3 below which is correct. Item 4 is the full blown new code and reference file, but with an old value for PCTETLEN.

  1. standard software, original calibration reference file (baseline result, parallel CTE)
  2. standard software, new calibration reference file with CTE_NAME/CTE_VER modified so code will choose the latest generation CTE and keyword PCTETLEN set to the old value of 60 for comparison to old data (make sure new calibration file still yields the same results as the old for parallel CTE)
  3. new software with serial processing disabled, new calibration reference file with PCTETLEN=60 (make sure new logic does not modify basic processing for parallel CTE),
  4. new software with both serial and parallel CTE corrections done, new calibration reference file with CTETLEN=60 (full functionality)

While I have identified the particular object in the image which is discussed in the paper, I cannot say if I see the improvement. I can make the data and software available to you. I have not yet optimized the code, and I may change the way the values are read from the new calibration reference file, depending on the final number of new extensions you want. In any case I wanted you to know I have been working this issue.

@jryon
Copy link
Author

jryon commented Oct 16, 2023

@mdlpstsci Thanks for this great progress! I was out on vacation and then sick for a couple of days, so it'll take me a bit to catch up, but I'm working on it.

@jryon
Copy link
Author

jryon commented Oct 17, 2023

@mdlpstsci Sounds like your testing so far is going well. I'd be happy to take a look at the image you used for testing. It may be that the preliminary PCTETAB I sent you contains an arbitrary serial parameter set, I can't recall. So there may not be much of a correction to see, but I'll take a look.

I've put together the current best set of parameters for serial and parallel corrections in a single PCTETAB, located here: /grp/hst/acs3/ryon/ctecorr/pinning/for_michele/combined_par_ser_pctetab.fits
You should have access to that directory, but please let me know if not. This PCTETAB has 20 extensions, and I've tried to add/correct the header keywords you recommended in the comments above to make it clear which extension is for which amplifier and readout direction. Let me know if I've missed anything.

I'll note that this is likely not the final version of the PCTETAB. In my testing, I'm seeing some residual serial CTE trails in pre-2022 data that I can't really explain, so it's possible the number of extensions in the PCTETAB may change again. Hopefully this won't cause too much of a headache for you.

It would be great if you could test CALACS with this new PCTETAB. I have a number of dark frames we can compare to, so it'd be great if you could run the corrections on any of these files: /grp/hst/acs3/ryon/ctecorr/pinning/serial_data/2022/2022????_long_dark_crrej.fits

@mdlpstsci
Copy link
Contributor

@jryon Thanks for the update. It will take me a few days to get to this as I have to fix a WFC3 issue for software which is part of the latest Release Candidate. I will get to this as soon as I can.

@mdlpstsci
Copy link
Contributor

@jryon I copied your new PCTE TAB calibration file, and I see you have made changes I suggested to the headers. I will now update the code to collect data from this new file which should keep me busy for a bit. Once I am done, then I will attempt to test with data. I will keep you updated.

@mdlpstsci
Copy link
Contributor

@jryon I can properly read your latest PCTETAB file. There is one small issue in that the CTE_VER FITS keyword needs to be a string and not a float.

I have to modify the way I am applying the corrections in the code, and this will take me more time.

@jryon
Copy link
Author

jryon commented Nov 7, 2023

@mdlpstsci Thanks, I will make that update to CTE_VER. No problem, I'm still working out the best parameters for past datasets, so take your time.

@mdlpstsci
Copy link
Contributor

mdlpstsci commented Jan 24, 2024

@jryon
I have been working on the serial CTE correction update and fixed a number of bugs associated with my reorganization of the code. I am using the *crrej.fits files you provided above for testing. While it is easy for me to see the parallel CTE correction, I have a much harder time seeing the serial CTE correction. I am running my code in a hacked version where only the serial CTE is being performed, and I do see some differences. For the *crrej.fits files above as displayed in ds9, can you give me some pixel locations (a location for each amp) where the serial CTE is obvious and I should see things improve after the correction has been applied. Ideally, this would be where ONLY the serial CTE correction has been applied. I need to make sure the code is working properly as I fooled myself previously. THANKS.

@jryon
Copy link
Author

jryon commented Jan 25, 2024

@mdlpstsci Great! I'd suggest looking at very hot pixels (>~10k e- in the crrej files) near the amp gaps, so between x ~ 1900 - 2150 in both chips. The highly saturated hot pixels should also have obvious trails. Their approx locations: WFC2 - (1868, 1599), (2703, 1428), (2736, 1592); WFC1 - (2576, 551), (799, 211)

Don't know how familiar you are with ds9, but here's what I typically do to see the trails/corrections: open the corrected file in one frame and the uncorrected file in a second frame, then set both to the same scaling (say 0 to 1000e-), lock them according to image coordinates, and then blink the frames to see the changes.

@mdlpstsci
Copy link
Contributor

mdlpstsci commented Jan 26, 2024

@jryon
Thanks for the information, but this is already what I have been doing! I just do not see much improvement with the serial correction in my testing. I understand both the serial CTE and its improvement may be much more subtle than the parallel correction which is clearly improved. Aside from the CRREJ images discussed above, I have also processed an image which Yotam discusses in the ISR ACS 2020-07(jdg301bwq), and I use the same hot pixel region indicated in the ISR for evaluation. It is easy to conclude that I either have a bug in my code (most likely), and/or I have an outdated calibration reference file. The one I am using is from the October 17, 2023 discussion. Do you have a newer file? Also, do you actually have some output from your testing so I can see how much of an improvement to expect? I would need both the input and output files for this. Many Thanks.

@jryon
Copy link
Author

jryon commented Jan 26, 2024

@mdlpstsci Ah I see. I have newer PCTETAB file(s), one per amp, that I'll combine into a single file for you today. I think the 2022 correction should be about the same with these newer files, but your tests will tell.

I've made a few animated gifs blinking uncorrected and corrected darks with the newest parameters:

2022-01-07 dark, WFC2:
/grp/hst/acs3/ryon/ctecorr/pinning/serial_results/uncorr_corr_20220107_ampdep_dec2023.gif
2022-01-07 dark, WFC1:
/grp/hst/acs3/ryon/ctecorr/pinning/serial_results/uncorr_corr_20220107_wfc1_ampdep_dec2023.gif
2015-02-12 dark, WFC2:
/grp/hst/acs3/ryon/ctecorr/pinning/serial_results/uncorr_corr_20150212_wfc2_ampdep_dec2023.gif
2015-02-12 dark, WFC1:
/grp/hst/acs3/ryon/ctecorr/pinning/serial_results/uncorr_corr_20150212_wfc1_ampdep_dec2023.gif

The files I'm blinking for these two sets of gifs are here:
2022 uncorrected: /grp/hst/acs3/ryon/ctecorr/pinning/serial_data/2022/20220107_long_dark_crrej_flt.fits
2022 corrected: /grp/hst/acs3/ryon/ctecorr/pinning/serial_data/2022/iter2_iter1_unrot_comb_20220107_long_dark_crrej_flc.fits
2015 uncorrected: /grp/hst/acs3/ryon/ctecorr/pinning/serial_data/2015/20150212_long_dark_crrej_flt.fits
2015 corrected: /grp/hst/acs3/ryon/ctecorr/pinning/serial_data/2015/iter2_iter1_unrot_comb_20150212_long_dark_crrej_flc.fit

I'd also be happy to take a look at your files if you think that'd be useful.

@jryon
Copy link
Author

jryon commented Jan 26, 2024

@mdlpstsci The most updated PCTETAB is here: /grp/hst/acs3/ryon/ctecorr/pinning/serial_results/combined_par_ser_pctetab_jan2024.fits

@mdlpstsci
Copy link
Contributor

@jryon
I have built my development version of the CALACS/ACSCTE code on the Linux systems. This version only applies the serial CTE correction so that I can evaluate if the code is performing properly. This code reads your latest version of the new PCTETAB file.

The calacs.e/acscte.e executables can be accessed here:
/home/mdelapena/acs_repo/hstcal/bin.Linux-3.10.0-1160.105.1.el7.x86_64-x86_64-with-glibc2.17

I have a result in /home/mdelapena/ACS where I did
$ acscte.e 20220107_long_dark_crrej.fits
and got the result
20220107_long_dark_crrej_blc_tmp.fits

When I blink my input against my output blc, it looks to me as though my version of the code is overcorrecting. It may look this way if there is an "off by 1 pixel" issue somewhere. I would appreciate your comments.

@jryon
Copy link
Author

jryon commented Jan 30, 2024

@mdlpstsci
It does look like your code is overcorrecting. I'm not sure if it could be an "off by one pixel" issue, since the first pixel in the trail should be the most corrected, as your code is clearly doing. The hot pixel should be increased in value after correction, and that is happening too.

Interestingly, the problem appears to be mostly contained to the first pixel in the trail. I did a few spot checks of hot pixels in my corrected crrej files and yours, and the +2, +3, etc. trail pixels are agreeing well. I'm not sure if that tells us much though, since the trail profile drops off very quickly after +1.

I'll continue looking into this tomorrow!

@jryon
Copy link
Author

jryon commented Jan 31, 2024

@mdlpstsci
I took a look at the trailer file from your processing, and I think I see the issue. I forgot that I had changed the CTEDATE0 and CTEDATE1 header keywords for serial CTE (along with some others) that are key to the scaling of the correction, and these didn't make it in to the PCTETAB I gave you. I haven't thought about how best to incorporate those into a combined PCTETAB with both parallel and serial parameters, since those keywords are staying the same for the parallel direction.

For current testing, since you're only doing the serial correction, I'll update the keywords in the primary header and send you the new file. Sorry for the oversight!

@mdlpstsci
Copy link
Contributor

@jryon
No problem at all. I am still worried there is another issue, but one step at a time. If necessary, you can have CTEDATE0/CTEDATE1 be amp-dependent for the serial correction. I have the parameters for the parallel and serial corrections in separate internal parameter structures.

@mdlpstsci
Copy link
Contributor

@jryon
So I can use your file as the truth, where is your jdg301bwq_flc.fits?

@jryon
Copy link
Author

jryon commented Feb 9, 2024

@mdlpstsci No need to apologize. My fault for assuming they were ready!

/grp/hst/acs3/ryon/ctecorr/pinning/michele_testing/ryon_processed/iter2_iter1_unrot_comb_jdg301bwq_blv_tmp_flc.fits

@mdlpstsci
Copy link
Contributor

@jryon
I overlooked updating PHDU History keywords which still apply to the parallel-only CTE case, so I need to fix this for the output files.

  1. For the iter2_iter1_unrot_comb_jdg301bwq_blv_tmp_flc.fits file, I just want to make sure you have ONLY applied the serial and not any parallel CTE correction to the data. When visualizing the image, this seems to be the case, but I want to make sure.
  2. Also, it looks like somehow I have a different version of the RAW than you do (i.e., at least a different CCDTAB specified), so I will wait to get confirmation from you on the file above before I try more testing as I need to make sure we are starting with the same RAW file. As such, I want to make sure I have your RAW and FLC file for jdg301bwq.

@jryon
Copy link
Author

jryon commented Feb 12, 2024

@mdlpstsci

  1. Yes, only the serial correction has been applied to that file.
  2. Hmm, I'm seeing the same CCDTAB in my RAW and processed file as compared to what is in your /home/mdelapena/ACS directory. The RAW I used is here: /grp/hst/acs3/ryon/ctecorr/pinning/michele_testing/ryon_processed/jdg301bwq_raw.fits

@mdlpstsci
Copy link
Contributor

@jryon
Sorry to take so long. Please try using the executables located here:

/home/mdelapena/cte_repos/hstcal/bin.Linux-3.10.0-1160.108.1.el7.x86_64-x86_64-with-glibc2.17/calacs.e (or acscte.e)

which is for test purposes only. This version only has the X CTE correction in play. The two comparison images are located in
/home/mdelapena/ACS/Gen3CTE_Xonly

@jryon
Copy link
Author

jryon commented Feb 28, 2024

@mdlpstsci
No worries. Real quick - I took a look at /home/mdelapena/ACS/Gen3CTE_Xonly/jdg301bwq_flc.fits and I'm seeing similar individual negative pixel effects as before. Could the rotation issue still be present?

@mdlpstsci
Copy link
Contributor

@jryon
Sorry to keep going around and around with you on this. When I display /home/mdelapena/ACS/Gen3CTE_Xonly/jdg301bwq_flc.fits, I see (I tried to capture the same areas you posted three weeks ago. The negative values look similar to your image to me. However, I take it when you display the image you are not seeing what I have posted here? I will say that when I difference this image against iter2_iter1_unrot_comb_jdg301bwq_blv_tmp_flc.fits the number of differences I get in each SCI are < 4%.

Screen Shot 2024-02-28 at 2 08 51 PM
Screen Shot 2024-02-28 at 2 10 34 PM

I have fixed the rotation issue, but I have been distracted by the Linux and Mac results not agreeing. But I would like to work one issue at a time. When you display the image, do you see what I see?

@mdlpstsci
Copy link
Contributor

If you run the calacs.e executable on the jdg301bwq_raw.fits, does the output match my jdg301bwq_flc.fits?

@jryon
Copy link
Author

jryon commented Feb 28, 2024

@mdlpstsci
Some regions of our two images match exactly, and others have these residual negative pixels in your FLC (right panel is your FLC, left is mine):
image
image
I made a diff image and there I see that the regions that match exactly are the left half of amp A, the top half of amp B, the top half of amp C, and the left half of amp D.
image
I'll try running the executable next.

@mdlpstsci
Copy link
Contributor

mdlpstsci commented Feb 28, 2024

@jryon
Thank you very much Jenna. Noting the amps and the portion of the amps is a big help!

@jryon
Copy link
Author

jryon commented Feb 28, 2024

@mdlpstsci
When I run the calacs.e executable, I get a different distribution of negative pixels in the resulting FLC than in your FLC, but they're still present and in the same portions of the amps. I'm running the executable on plhstins4. Weird!

image

image

@mdlpstsci
Copy link
Contributor

@jryon
Sorry for the retries. I still have a problem between the Mac and Linux. However, it is most important the Linux results match your results as Linux is the OS where the official pipelines execute. Please find in /home/mdelapena/FOR_JENNA/jdg301bwq_flc.fits which is the latest output I generated based upon the executable noted in the next sentence, as well the associated trailer file. This output has ONLY the Serial CTE correction applied. The SCI extension data matches your data, also located in this directory. The executable which created this data can be accessed in /home/mdelapena/hstcal/bin/calacs.e. You should be able to use this executable for testing. If you have any issues at all, please let me know.

Just as an FYI - the source is actually located in /home/mdelapena/fresh_repo/hstcal.

I thought I would give you access to these results while I puzzle over the OS-mismatch. As an FYI - this data has been created with a version of the HSTCAL package which has been reorganized by the build manager to clean up cruft, remove circular dependencies, and replace the use of WAF with CMAKE.

@jryon
Copy link
Author

jryon commented Mar 4, 2024

@mdlpstsci
Just curious - have you or anyone ever tested Linux vs Mac for the existing parallel CTE correction? Naively, it seems strange to me if the serial correction has an OS mismatch but the parallel one does not.

@mdlpstsci
Copy link
Contributor

@jryon Yes, I have done modest testing for the specific update to incorporate the serial correction as I need to make sure that my changes did not modify anything in the parallel processing. The parallel processing is fine. I think I have found a subtle issue associated with OpenMP in the new code, so I believe I have fixed the "serial" problem. I need to do more testing.

@jryon
Copy link
Author

jryon commented Mar 4, 2024

@mdlpstsci
I've confirmed that your latest jdg301bwq_flc.fits matches mine exactly. I also ran the executable on the 20220107 long dark and found exact match to one I processed, so everything is looking good!

@mdlpstsci
Copy link
Contributor

@jryon I would like to make sure that I have the latest version of the 20220107 file. Is this the latest input and output?

INPUT
files: /grp/hst/acs3/ryon/ctecorr/pinning/serial_data/2022/2022????_long_dark_crrej.fits

OUTPUT
/grp/hst/acs3/ryon/ctecorr/pinning/serial_data/2022/iter2_iter1_unrot_comb_20220107_long_dark_crrej_flc.fits

@jryon
Copy link
Author

jryon commented Mar 5, 2024

@mdlpstsci
Thanks for double-checking. The input is correct, but the latest output is here:
/grp/hst/acs3/ryon/ctecorr/pinning/serial_data/test_ctedate0_2022/iter2_iter1_unrot_comb_20220107_long_dark_crrej_flc.fits

I should note that I use a custom CCDTAB for processing these long darks since they are stacks of individual dark frames, so their readnoise is reduced by the square root of the number of darks. The custom CCDTAB is in the same directory above, and (confusingly, sorry) has the same name as the reference file: 72m18219j_ccd.fits

@mdlpstsci
Copy link
Contributor

@jryon
I am clearing doing something wrong as I cannot get my output on any 20220nnn_long_dark_crrej.fits file to match your output, but I know I had agreement in the past. I changed the header to PCTECORR=PERFORM, modified the CCDTAB to force the use of your CCDTAB, and set PCTETAB="combined_par_ser_pctetab_jan2024.fits".

Having said this, my jdg301bw results agree your results. If you cannot think of something that I left out, can you please processing a few more of the darks with you method and my executable?

I did not want to overwrite the executable in /home/mdelapena/hstcal/bin/calacs.e which still exists. My newest executables live here: /home/mdelapena/fresh_repo/hstcal/_build/pkg/acs. You should get the same results regardless of the executable used, but I wanted to be careful.

Once you are satisfied the serial CTE looks fine, then I will re-enable the parallel CTE and we can run fuller tests. THANKS

@jryon
Copy link
Author

jryon commented Mar 6, 2024

@mdlpstsci
I think I've been confusing myself and by extension you, sorry about that! It looks like I was able to get perfect agreement between BLC_TMP 20220107 long darks that I processed with the normal CCDTAB in jref, one file with my code and one file with your executable /home/mdelapena/hstcal/bin/calacs.e. These live here:
My code: /grp/hst/acs3/ryon/ctecorr/pinning/michele_testing/20220107_long_dark_crrej_blc_tmp.fits
Your executable: /grp/hst/acs3/ryon/ctecorr/pinning/michele_testing/ryon_processed/iter2_iter1_unrot_comb_20220107_long_dark_crrej_blc_tmp.fits

I will also start from scratch and process a few darks all the way to FLC, ensuring the same reference files are used, and let you know what I see.

@jryon
Copy link
Author

jryon commented Mar 6, 2024

@mdlpstsci
Ok, I processed these with /home/mdelapena/hstcal/bin/calacs.e:
/grp/hst/acs3/ryon/ctecorr/pinning/michele_testing/new_calacs_processed/2022*_long_dark_crrej_flc.fits

And these with my code:
/grp/hst/acs3/ryon/ctecorr/pinning/michele_testing/ryon_processed/iter2_iter1_unrot_comb_2022*_long_dark_crrej_flc.fits

Both sets use the normal CCDTAB in jref (for simplicity) and the same PCTETAB (combined_par_ser_pctetab_jan2024.fits). I'm seeing exactly the same SCI arrays in both datasets.

At this point, I'm happy to sign off on the serial CTE step in your executables.

@mdlpstsci
Copy link
Contributor

Super - I will now enable the serial + parallel CTE correction, and I will let you know when the executable is available.

@mdlpstsci
Copy link
Contributor

@jryon
I have enabled the parallel CTE correction so both the serial and parallel CTE corrections are being applied in the latest version of the code. You can find the executables for testing here:

/home/mdelapena/fresh_repo/hstcal/_build/pkg/acs

FYI: IMHO I would update the DESCRIP keyword in the new calibration file primary header from Parameters needed for gen2 pixel-based CTE correction to Parameters needed for the serial plus parallel pixel-based CTE correction or something like this so all the documentation in the header of the file is consistent.

@jryon
Copy link
Author

jryon commented Mar 8, 2024

@mdlpstsci
I've processed one file with the new executable, and based on the terminal output, it looks like the parallel CTE correction step is run before the serial step. Can you confirm that's the case? If so, they should be swapped so serial comes before parallel.

@mdlpstsci
Copy link
Contributor

@jryon
Sorry that it is misleading. The parallel information is read first and stored in a "parallel" structure as the same info is used for every amp. Then serial information is read as it is needed into a "serial" structure. Only one set of serial information is available (or needed) during the processing of the associated amp.

@jryon
Copy link
Author

jryon commented Mar 8, 2024

@mdlpstsci
Ah ok, I didn't look carefully enough at the subsequent lines - I see where it says performing the two types of correction, and they're in the correct order. Thanks!

@jryon
Copy link
Author

jryon commented Mar 14, 2024

@mdlpstsci
I've processed 3 long dark datasets and 1 external stellar field with your serial + parallel correction executables, and found that they agree exactly with the same files processed with my code, both serial and parallel correction. I'm ready to sign off on the CALACS implementation of this.

@jryon
Copy link
Author

jryon commented Mar 20, 2024

@mdlpstsci
I realized I forgot a key component of this. We want the serial CTE correction to run on post-SM4 data only. The parallel correction should continue to run on both pre- and post-SM4 data. I think it makes sense to implement this as some kind of switch inside CALACS rather than having two operational PCTETABs with different USEAFTERs (since that might require some logic inside CALACS anyway?).

Sincere apologies for overlooking this until now! Let me know if there's any more info you need.

@mdlpstsci
Copy link
Contributor

@jryon
This should not be an issue, but I will have to take a look and see how other pre- and post-SM4 situations have been handled for consistency.

@mdlpstsci
Copy link
Contributor

mdlpstsci commented Apr 15, 2024

Because this issue is temporarily being put on hold until Jenna has the opportunity/time to examine the regression test data produced (once the software is merged), this is a reminder for me.

  1. Implement the requirement for the serial portion of the CTE to be applicable only for post-SM4 observations. DONE (Jira HLA-1241, 07 May 2024)
  2. My on-disk repo is fresh_repo and the branch is acs_serial_cte. Test input and output data, as well as the new calibration reference file generated by Jenna, combined_par_ser_pctetab_jan2024.fits, is in ACS/CTE.

@mdlpstsci
Copy link
Contributor

@jryon Since there are already places in the CALACS code which check for pre- and post-SM4, I just used an "if" statement to check if the expstart (as done in other locations in the source) is "<" or ">= SM4MJD" where SM4MJD=54967.

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

No branches or pull requests

2 participants