Update emicorr to run on FAST mode MIRI data #8264
Comments
|
Comment by Maria Pena-Guerrero on JIRA: The code was set up to run any time the string 'FAST' or 'SLOW' is in the read pattern, in other words the step is set up to run for both FAST and FASTR1 and SLOW and SLOWR1. The reference file was designed with this in mind. Questions: Are you expecting to have 2 or more reference files for read patterns, e.g. one file FAST/SLOW and another for FASTR1/SLOWR1?There is a default on-the-fly calculation that happens if CRDS returns 'N/A' for reference file. Should this be disabled and the step skipped? |
|
Comment by Misty Cracraft on JIRA: I'd like Eddie Bergeron to weigh in, since he's the expert on behavior of the noise and how to correct it. I don't have specific expectations on how any of this should work. FAST mode and FASTR1 modes are different, varying by a reset frame in between integrations. I'm not sure what the differences are in the noise patterns to be corrected. We do not want the same correction being applied to FAST and FASTR1 data. We don't want any correction at all applied to any FASTGRPAVGn data. In the previous ticket, Eddie seemed to think it would be simple enough to change the code to run for FAST mode, but I'm not sure if that's based on the code as written in jwst, or as it was in Eddie's code, so I'm going to leave that for him to answer.
|
|
Comment by Maria Pena-Guerrero on JIRA: Misty Cracraft what about the default correction, should that be turned off unless requested by the user? Also, I made the reference file with this in mind so the read patterns there read FAST and SLOW. You should change them to FASTR1 and SLOWR1 according to what you wrote above. |
|
Comment by Eddie Bergeron on JIRA: The reference waveforms should be the same for both FAST and FASTR1, so the existing reference file is fine as-is. Both FAST and FASTR1 can use the existing FASTR1 reference waves in the reference file. However I will try to confirm this. The only change is to the pixel phase calculation. As Misty mentioned, its just a matter of with/without the time for the extra reset frame between integrations. A simple "if" test keying off of either FAST or FASTR1 is all that is needed in the phase calculation (same for idl and Maria's code). I'll dig up the necessary line and post it here. I'd like to test it first to make sure it works as expected. So really there are two code changes needed:
|
|
Comment by Eddie Bergeron on JIRA: For the default correction (when the CRDS reference file is not found), my preference would be to turn it off rather than use a default set of frequencies stored someplace else. Those frequencies could change as we improve them (for example, I've found that the 10hz frequency is not the integer multiple of the master clock that was expected - it differs by about 400 ppm) and I want to avoid having to keep track of two separate sets of frequencies. In any case, using the self-correction can be risky (noisy) for sets with limited numbers of frames. I'd rather the default pipeline do nothing if it can't find a reference file, which should never happen anyways. As long as users still have the option of self-correction when they run the pipeline themselves (by passing the desired frequency or frequencies to phase as a parameter) that should be sufficient. But this decision is up to the MIRI team. |
|
Comment by Eddie Bergeron on JIRA: I think it's ok to use just "FAST" and "SLOW" in the reference file - the way you have it now. I suspect the noise waveforms themselves will be the same for all FAST* patterns, so it's more proper the way you have the reference file set up. However I will try to confirm this on some test data. I haven't looked at any "FAST" exposures yet. |
|
Comment by Maria Pena-Guerrero on JIRA: For the on-the-fly correction, in addition to be given the frequency, the code also needs to know the rowclocks and frameclocks, so this would have to be a dictionary in the pipeline code. |
|
Comment by Eddie Bergeron on JIRA: Misty, out of curiosity, is the dataset you tested with "FAST" (the one that wasn't corrected properly) single-int or multi-int? Technically single-int "FAST" exposures should already work properly since the timing difference that would induce the error is the extra frame between ints. I see in the archive that there are 918 total exposures that used FAST. Of those, only 173 are multi-int. I'm surprised TAs would use multi-int. |
|
Comment by Eddie Bergeron on JIRA: We'll always have to carry that dictionary for rowclocks and frameclocks, and that will only have to be updated if MIRI ever adds new subarrays. However we might add or change frequencies over time (e.g. the "other" frequencies such as the 164Hz associated with 390Hz for some subarrays). I'd prefer that the pipeline frequencies only exist in the reference file. |
|
Comment by Misty Cracraft on JIRA: Eddie, the file I tested does seem to be single int, but the way I know something went wrong is that when the shower code was run on it, it overflagged most pixels in the coron array, which is what I saw for all coron data before the emicorr was added to the pipeline. jw01194007001_02101_00001_mirimage_uncal.fits MIRI MASK1065 FND 1 44 FAST The file was single int, 44 groups, and the only one of the coron data that still had the overflagging. Can you use this file as one of your FAST mode test files? |
|
Comment by Eddie Bergeron on JIRA: Ok, thanks. 44 groups in that single int should be sufficient for a good phase match. I'll take a look today. |
|
Comment by Maria Pena-Guerrero on JIRA: Misty Cracraft and Eddie Bergeron I believe this is the behavior you want. Can you please test this branch to verify? pip install git+https://github.com/penaguerrero/emicorrfix```
|
|
Comment by Eddie Bergeron on JIRA: This is the line from the idl that needs to be conditional on FAST vs FASTR1: start_time=start_time+frameclocks ; add one frame time to account for the extra frame reset between miri integrations Couldn't remember if it was commented well enough, but looks like it was. I can't easily test your python version but I will modify my idl code for this conditional and test it there. |
|
Comment by Maria Pena-Guerrero on JIRA: do you mean if readpatt=='FAST' then do start_time=start_time+frameclocks```
elif readpatt=='FASTR1' do?
else? |
|
Comment by Eddie Bergeron on JIRA: The other way around. That is, add frameclocks if "FASTR1", but do not add frameclocks if "FAST". i.e. if readpatt=='FASTR1' then start_time=start_time+frameclocks else don't do this.
|
|
Comment by Eddie Bergeron on JIRA: Actually it should be for FASTR1 AND SLOWR1. So: if readpatt=='FASTR1' or if readpatt=='SLOWR1' then then start_time=start_time+frameclocks The "R1" in the readpatt moniker stands for "one extra reset frame between ints," and frameclocks is the amount of time it takes to do that reset. So in the FASTR1 or SLOWR1 cases (as is the default now), frameclocks should be added to start_time between ints here. If miri were ever to add an "R2" pattern (they won't) you would simply multiply frameclocks by that factor when adding. "FAST" is technically equivalent to "FASTR0" I don't know what FASTGRPAVG uses - I presume its R0 but right now it doesn't matter because we want to skip emicorr entirely for all FASTGRPAVG cases. That should also be put in the code. |
|
Comment by Maria Pena-Guerrero on JIRA: ok, thank you Eddie. This is implemented in the branch, as well as: a) emicorr will skip if the reference file returns as N/A or skip=True b) there is a new optional parameter called "onthefly_corr_freq" which is a list of the frequencies the user wants to do a correction on-the-fly. This on-the-fly reference file can be saved with save_intermediate_results=True Misty Cracraft do you want me to code a condition to skip the step if readpatt=='FASTGRPAVG'? |
|
Comment by Misty Cracraft on JIRA: Well, FASTGRPAVG could be one of several options: FASTGRPAVG, FASTGRPAVG8, FASTGRPAVG16, FASTGRPAVG32, or FASTGRPAVG64. We want to skip emicorr for all of those. |
|
Comment by Maria Pena-Guerrero on JIRA: so just run emicorr for FAST or FASTR1? (or SLOW and SLOWR1) |
|
Comment by Misty Cracraft on JIRA: Yes, run emicorr for FAST, FASTR1, SLOW (not sure any exists), and SLOWR1. Do not run for any iteration of FASTGRPAVG. If you don't want that hardcoded, we can put in an emicorr parameter reference file to run for specific cases and skip specific cases. The conditional FAST/FASTR1 exists to make sure the correct reset time is used in any place it's needed. But no, we don't intend to run it on FASTGRPAVG.
|
|
Comment by Eddie Bergeron on JIRA: I would explicitly hardcode the block on FASTGRPAVG* in the code itself since there is no way emicorr will work on that until/unless code is written to handle it. This will avoid accidental application of emicorr from outside and set a flag to any future developers who look at the code. Maybe add a comment to this effect in the code too. I was just searching the archive for miri data and noticed there is exactly 1 "SLOW" dataset in the archive: jw01173005001_02101_00001_mirifulong_dark.fits - a dark taken early in commissioning. There is also only one FASTGRPAVG16 - a TA taken recently, and no FASTGRPAVG32, or FASTGRPAVG64. Still best to exclude all FASTGRPAVG* in case there are future ones. |
|
Comment by Maria Pena-Guerrero on JIRA: Ok, I think we are good. The code has this block now:
allowed_readpatts = ['FAST', 'FASTR1', 'SLOW', 'SLOWR1']
if readpatt.upper() not in allowed_readpatts:
step will be skipped```
|
|
Comment by Eddie Bergeron on JIRA: Hey Misty, I just ran your test set (jw01194007001_02101_00001_mirimage_uncal.fits) through my original emicorr code - i.e. without any fix for FAST vs FASTR1. Since this is a single-int 44 group exposure it should work fine, and it did. The 390Hz noise was removed correctly. I've uploaded a figure showing the self-phased wave and with the reference wave overplotted in red. Lower left shows CDS images corrected and uncorrected. So I think the issue you are seeing with the shower code is unrelated to the emicorr. I noticed a couple of oddities with this file though. 1). There's a saturated snowball in the first group. 2) I get a warning from the idl fits_write routine when trying to write out the ASDF extension of this file with the corrected version of the data: % FITS_WRITE: ERROR: A Byte array must be supplied with a BINTABLE or TABLE extension I tried a few other "FAST" mode single and multi-int exposures and those are not throwing an error on fits_write, so maybe there's a problem with this individual file. Can you test a few other "FAST" exposures? Also, I noticed that all the TACQ exposures are indeed single integration, FAST or FASTGRPAVG*. I kindof expected that to be the case. So emicorr unmodified 10.1 should already work on all of the single-int FAST exposures. (the few multi-int TACONFIRMs in the archive (5ea) are all FASTR1) However there are a number of multi-int FAST exposures - looks like about 40 exposures give-or-take, that are either cal, science or darks. These are from just 4 programs (1523, 1546, 1012, 1173). So the 10.2 fix we're talking about here for FAST vs FASTR1 should only affect those ~40 datasets. I'll test a few of those to see what they look like. Test cases for the new code have to come from this set of 40 to confirm it works. |
|
Comment by Eddie Bergeron on JIRA: Of these ~40 multi-int FAST datasets, ~40 are FULLFRAME, 5 are BRIGHTSKY and 3 are SUB256. These are all in-phase for 390Hz so this leaves only an improvement in the 10Hz correction for these 40 datasets with the new FAST/FASTR1 timing. This will be harder to detect in testing due to the lower 10Hz amplitude, but we should be able to see it. |
|
Comment by Eddie Bergeron on JIRA: ah, I missed ~30 multi-int FAST datasets that were scrolled off the screen. These are all MASK* subarray datasets, so these can be used to look at the 390Hz across ints. I had a look at both the 390Hz and 10Hz in these cases using my IDL version of emicorr. The code fix for FAST vs FASTR1 does indeed solve the problem. I'll post a list of example datasets you can use to test the python against tomorrow. Also I think the fits_write error I'm seeing on Misty's original single-int example is just because it's a TACQ. I'm getting the same error on other TACQs, so the issue there is probably an extra binary table in that ASDF extension. I'm guessing the shower code problem with that dataset is due to that saturated snowball in the first group. Mike should be able to sort it out. |
|
Comment by Eddie Bergeron on JIRA: Here's a set of readpatt="FAST" test cases for the pixel phase calculation fix, both single and multi-int here. The new fix should only affect the multi-int ones. Some FASTGRPAVG* sets as well to test that they are skipped: {panel} I've attached some figures showing the PA wave and corrected vs uncorrected FAST CDS for both 390 and 10Hz. Also a comparison of a groupaveraged vs non-groupaveraged FAST CDS image showing no need for emi correction in the FASTGRPAVG* cases. I wonder if anyone has compared fastgroupaveraging to slowmode for noise performance? I don't think groupaveraging vs correcting 390hz is a noise win for subarrays (better to fit and remove it rather than average it down), but there might be advantages for fullframe slow (which has no 390Hz) depending on the noise power spectrum. If you can avoid the thermal issues of slowmode by using FASTGRPAVG8 (8-frame avg with ~same SLOWR1 frametime) or even FASTGRPAVG (4-frame avg, half the SLOWR1 frametime), groupaveraging might be a better option than SLOWR1 for reducing data volume in those situations. Especially if the noise performance is better.
|
|
Comment by Misty Cracraft on JIRA: Just checking on the status of this. For the first set of emicorr parameter reference files (aimed at build 10.2), we run emicorr on all FASTR1 and SLOWR1 data, and skip it for anything else. So I wondered if the FAST/SLOW updates are going to be in place for build 10.2 and if I need to update those reference files at some point. |
|
Comment by Maria Pena-Guerrero on JIRA: All of the changes you requested are in the branch ready for code review. The changes are:
allowed_readpatts = ['FAST', 'FASTR1', 'SLOW', 'SLOWR1']```
3. The step has a new optional parameter,"onthefly_corr_freq", in which the user can provide a list of frequencies to do a connection on-the-fly without a reference file.
4. The step will only add the frame clocks when the read pattern is either FASTR1 or SLOWR1, i.e.
```java
if readpatt.upper() == 'FASTR1' or readpatt.upper() == 'SLOWR1':
start_time += frameclocks ```
With all these changes the, the reference files work as currently in CRDS.
You can test this branch with:
pip install git+[https://github.com/penaguerrero/jwst@emicorrfix] |
|
Comment by Eddie Bergeron on JIRA: Note relatively poor match of 10hz reference wave to PA. I'm working on an updated reference file with better reference waves and hopefully the additional frequencies associated with 390. This refwave was preliminary just to get that first reference file built and tested. Even so, the correction works pretty well as-is, but there are residuals at the sub-DN level with this early 10Hz wave. The current reference file is good enough for the time being. |
|
Comment by Misty Cracraft on JIRA: I just ran a test with Maria's updated reference file. Eddie, the BRIGHTSKY and SUB256 data was part of that testing. If you want to check the results, the updated rate and diff files are in /ifs/jwst/wit/miri/cracraft/emitests/. |
|
Comment by Eddie Bergeron on JIRA: I checked your BRIGHTSKY one. No change from before. Looks about the same, again both phase and amplitude not corrected properly relative to the idl version, although the amplitude in the BRIGHTSKY case is closer than in the SUB256 case. |
|
Comment by Eddie Bergeron on JIRA: To check the correction, take the difference between a frame of your emidiff and noemidiff. Plot a column of that difference. The residual should have roughly the same amplitude as the reference wave (1.5 DN PtoP). The residual won't have the exact shape of the reference wave because its a difference at two phases, but the amplitude should be ~ 1.5 DN. I'm only seeing about half that amplitude in your result and you can visibly see that the correction isn't working when blinking an emi vs a noemi or uncal difference. |
|
Comment by Eddie Bergeron on JIRA: Running with a 10hz self-correction might rule in/out an issue with the reference file. I did check Maria's new reference file ([^jwst_miri_emicorr_0001_fix.asdf]) and the rowclocks/frameclocks values are now correct for BRIGHTSKY. Given that the phase seems to match in BRIGHTSKY, there might have been two overlapping problems rather than just the clocks in the reference file. Still worth trying a 10Hz self correction to see if that is working at least. If so, the scaling problem is downstream from the phasing. |
|
Comment by Misty Cracraft on JIRA: The coronagraphs group will be taking over this testing, likely Jonathan Aguilar , but I have a question about the subarrays. Are the issues with the BRIGHTSKY and SUB256 only seen with FAST mode data, or do we think the mis-phasing was a problem in the FASTR1 set as well? Eddie Bergeron Do you remember if you looked at those specific datasets with FASTR1? |
|
Comment by Maria Pena-Guerrero on JIRA: I have been carefully looking at the python code, running in parallel my copy of the IDL code printing stages in between and I do not see any differences that are too large. In other words, I think the Python code is fine as is in the branch. This is what I find: !jw01538_emi_comparison.png|thumbnail! !jw01546_emi_comparison.png|thumbnail! |
|
Comment by Maria Pena-Guerrero on JIRA: This is the idl code I have, in case you want to check with what you have Eddie Bergeron [^fix_miri_emi.pro] |
|
Comment by Eddie Bergeron on JIRA: It shouldn't be related to the FAST vs FASTR1 issue, but I can't say for sure without you either running Maria's latest version on some FASTR1 sets (maybe the one from the original ticket - https://jira.stsci.edu/browse/JP-3248 ?) and/or running this same FAST dataset with the integrations=1 parameter. I'd also recommend testing Maria's new self-calibration option, using the "onthefly_corr_freq" parameter From Maria's Feb 9 post above: |
|
Comment by Eddie Bergeron on JIRA: Maria, I think we need to check your python output against Misty's python output here to make sure they are the same. Since you are seeing no difference against the idl, and I am seeing differences between Misty's _emidiff.fits output and the idl. Need to make sure you are both running the same version with the same reference file and using the same parameters. |
|
Comment by Maria Pena-Guerrero on JIRA: I placed the files I produced with my version of the IDL code as well as the pipeline emi corrected and the uncorrected (the dq_init files) at central store location: █████████████████████████████████████████
[~bergeron] can run your checks on these files to confirm what I see?``` |
|
Comment by Misty Cracraft on JIRA: I have also re-run a test using Maria's emicorrfix branch and the updated reference file. The files are in the usual place, and to make sure Maria can access the files if needed, I'm also copying all of the emi related files to /user/cracraft/temp. There's a chance that the previous test I ran wasn't with the proper branch, so hopefully this shows improvement. |
|
Comment by Eddie Bergeron on JIRA: Ok, these now look good! Misty's BRIGHTSKY now matches Maria's and the idl results. The SUB256 case was working all along. The large difference between Misty's and IDL in the top attached plot was an error on my part - Misty's is the small 10hz residual in a CDS and the IDL version there is the absolute correction on an individual frame. The correction is difficult to diagnose in CDS because the SUB256 frametime is near an integer multiple of 10Hz. It's 29952/9960.94 = 3.00694 waves per frame, so it's almost 10Hz in-phase. The 10Hz noise is (surprisingly) accurately detected and phased with the model in this dataset. I guess there is just enough phase shift through each int for it to work, but it's small enough to be invisible by-eye in CDSs. If you look at wider differences such as group 200-20 the 10hz is easier to see. So the only problem was with the BRIGHTSKY case, and the fix to the clocks in the reference file dictionary (plus Maria's new branch for FAST vs FASTR1) has solved it. I also re spot-checked a few of the other sets just to make sure they still look ok. I think the last thing that should be checked is the functionality of the "onthefly_corr_freq" parameter. Need to make sure that works as expected since it is a change from 10.1. Maria has already tested it herself. Misty, can you run two datasets using this parameter? Try that last 5-int fullframe case with the 10hz frequency only, and the MASKYLYOT case with both 390Hz and 10Hz together: jw01523008001_02105_00001_mirimage_uncal.fits (10.039216 Hz) jw01532010001_02105_00004_mirimage_uncal.fits (10.039216 Hz and 390.62500 Hz) Keep your last version (the one made using the reference file wave) so you can compare them. Take the difference between the uncorrected and the two corrected (ref and onthefly) datasets - just one frame from the middle of each exp should be enough. You should recover the reference wave in one difference and the self-wave in the other. Plot a column though each difference image and compare to the red (ref) vs white (self) in the top attached plot. Should look about the same in the fullframe case, and will look weird in the MASKLYOT case because it'll be the sum of 10hz and 390hz, but I want to see that case run with two frequencies at the same time. |
|
Comment by Eddie Bergeron on JIRA: I ran these two examples through the idl code to show what those col plots should look like. Plots attached. These are 100-150 rows from a difference roughly in the middle of each exposure. If your plots look basically like these then the onthefly_corr_freq option is working. |
|
Comment by Howard Bushouse on JIRA: Fixed by #8270 |
|
Comment by Maria Pena-Guerrero on JIRA: Misty Cracraft just to confirm, is the default now to run the step? |
|
Comment by Misty Cracraft on JIRA: Right now, our detector1 parameter reference files run emicorr for FASTR1 and SLOWR1 readout patterns only. |
|
Comment by Eddie Bergeron on JIRA: With Maria's code changes for 10.2 described in this ticket, the default (from 10.2 on) should be to turn this on for "FAST" "FASTR1" "SLOW" and "SLOWR1," right? So presumably that means a mod to the detector1 parameter reference file with 10.2. Also Maria corrected a bug in the emicorr reference file dictionary for BRIGHTSKY. That new reference file needs to be delivered as well. This bug affects all BRIGHTSKY subarrays, including for FASTR1 as we are running now with 10.1. |
|
Comment by Howard Bushouse on JIRA: Additional bug fix in #8375 |
|
Comment by Misty Cracraft on JIRA: File [^jwst_miri_emicorr_0001_fix.asdf] has been delivered to CRDS OPS. |
|
Misty Cracraft Can we close this ticket out now? Presumably we'll also need to deliver new param ref files to CRDS enabling emicorr for FAST mode along with the 10.2 ops install? |
|
Comment by Misty Cracraft on JIRA: I believe we were leaving it up to coron group to confirm that the results for FAST mode were what they expected, but last I knew, Jonathan Aguilar hadn't had time to test it. I'm through with my testing, but I haven't seen any comments here by Jonathan to say that they've tested it. |
|
Over to you then Jonathan Aguilar |
|
Comment by Jonathan Aguilar on JIRA: Thanks Misty Cracraft and David Law ; working on it |
|
Comment by Jonathan Aguilar on JIRA: Hi all, yes the results for FAST mode look right for coronagraphy. There's a reduction in noise across the rows; it isn't very dramatic, but it's there. Only TA exposures use FAST mode; science exposures are taken with FASTR1. !image-2024-04-29-13-22-21-948.png|width=1063,height=343! |
Issue JP-3533 was created on JIRA by Misty Cracraft:
When testing the shower code with emicorr updates in build 10.1, I found a test case of a coronagraphic image that was still overly flagged (most of the image) as NaNs in the rate file. This file was in FAST mode. The emicorr code was only set up to run on FASTR1 and SLOWR1 data, which is the bulk of MIRI data, but TA data is taken in FAST mode and also needs to be corrected for emi, so that the MIRI team can properly calibrate coron data.
And we cannot turn on the shower code for coron data until the emicorr code properly treats FAST mode data.
We are not requesting that emicorr runs on FASTGRPAVG data, as Eddie pointed out that the group averaging beats down the noise and it would probably be more work to figure out how to correct it than the actual correction would benefit the data.
The text was updated successfully, but these errors were encountered: