spacetelescope / jwst

Python library for science observations from the James Webb Space Telescope
https://jwst-pipeline.readthedocs.io/en/latest/
Other
554 stars 161 forks source link

MIRI Subarrays 390 Hz EMI #7729

Closed stscijgbot-jp closed 8 months ago

stscijgbot-jp commented 1 year ago

Issue JP-3248 was created on JIRA by Michael Regan:

The majority of the MIRI subarrays have an 390 Hz EMI noise pattern in the raw data. This 390 Hz maps exactly to 256 pixels and has a constant amplitude of +- 4 DN. The effect of this is to imprint this into the rate images. For short integrations in LRSSLITLESS the correlated noise from this is quite apparent in the rate images. For longer integrations the net effect is to increase the read noise by ~20%.

The long term plan is a change to the sizes and locations of the subarrays to get the frame times to be in phase with the 390 Hz like the full frame images. For the previous and near term observations this can be fixed by adding a new step to the pipeline. A prototype has been implemented and shown to remove the 390 Hz to a fraction of a DN.

stscijgbot-jp commented 1 year ago

Comment by Eddie Bergeron on JIRA:

For some context, I've posted a short powerpoint presentation given to the ramps-to-slopes WG last fall with some info on MIRI 10Hz and 390Hz noise. It has been determined that the period of the 390Hz waveform is precisely 256.0 pixel times, and a number of different subarray size/location/frametime combinations close to the existing subarray dimensions have been derived that will eliminate the 390Hz noise operationally.   

stscijgbot-jp commented 1 year ago

Comment by Eddie Bergeron on JIRA:

Added a few more figures showing 390Hz noise in several of the existing MIRI subarrays, with some extracted waveforms and in/out of phase examples.

stscijgbot-jp commented 1 year ago

Comment by Misty Cracraft on JIRA:

Anton Koekemoer  Should this be presented at the next CALWG meeting? It should only affect MIRI, but it is a new addition to the pipeline being suggested.

stscijgbot-jp commented 1 year ago

Comment by Anton Koekemoer on JIRA:

Thanks very much Misty - indeed, I've been in discussion with with Mike and Eddie since they suggested this to me as a topic. (I also now added myself and also Greg Sloan as watchers on this ticket).

It might perhaps be efficiently scheduled as an 'out-of-cycle' CalWG mtg, ie not necessarily involving the monthly full CalWG since it's pretty MIRI-specific. though of course we'd advertise it to the full CalWG as always. Here are the dates I'd proposed to Mike and Eddie at the time when we first discussed this:

.. in terms of 'off-cycle' slots, the next one would be Jun 27 (since Jun 20 is actually taken already).

Then in July we have a full Cal WG meeting on Jul 11, and off-cycle slots available on Jul 18 and 25.

And finally in Aug we have a full Cal WG meeting on Aug 1, then with off-cycle slots available for any of the remaining Tuesdays until the next full Cal WG meeting on Sep 5, etc. 

So as soon as we hear from  Michael Regan and Eddie Bergeron about which date works best for them then we can schedule it for discussion.

 

stscijgbot-jp commented 1 year ago

Comment by Anton Koekemoer on JIRA:

Mike emailed me a few weeks after the above and this has now been scheduled for discussion at JWST Cal WG Meeting 2023-07-18

stscijgbot-jp commented 1 year ago

Comment by Anton Koekemoer on JIRA:

This was discussed at JWST Cal WG Meeting 2023-07-18 and approved as a candidate for implementation (following prioritization by the MIRI Instrument team as indicated in the criticality field for the ticket), please continue further discussion about implementation details/ questions in the comments here in this ticket.

stscijgbot-jp commented 1 year ago

Comment by Howard Bushouse on JIRA:

Eddie Bergeron can you provide SCSB with a copy of your IDL code that implements this correction?

Also, it was mentioned during the Cal WG discussion that Eddie's code could either derive a reference waveform from the exposure being processed or have it provided in a reference file. It seemed like the consensus for pipeline implementation was to use the reference file approach. Should we, however, keep the ability to generate a waveform from the data in the code that we implement, just in case it's useful in the future (or as an option for "power" users)?

stscijgbot-jp commented 1 year ago

Comment by Anton Koekemoer on JIRA:

Howard Bushouse  wrote:

Also, it was mentioned during the Cal WG discussion that Eddie's code could either derive a reference waveform from the exposure being processed or have it provided in a reference file. It seemed like the consensus for pipeline implementation was to use the reference file approach. 

Yes, the pipeline implementation would use the reference file approach.

Should we, however, keep the ability to generate a waveform from the data in the code that we implement, just in case it's useful in the future (or as an option for "power" users)?

From Eddie's presentation it sounded as though this may be implemented as two separate pieces, ie one to generate the waveform (either from the current data or some other set of data) which could remain as a stand-alone script if not in the pipeline, while the other would be the pipeline modification to remove the waveform from the data (regardless of whether it's being supplied as a reference file or generated on-the-fly from the same dataset).

So this question should be discussed with more input from the MIRI team (tagging Sarah Kendrew and the other MIRI people on this ticket), with a few considerations being:

stscijgbot-jp commented 1 year ago

Comment by Anton Koekemoer on JIRA:

hi Howard Bushouse just checking whether you've received the IDL code from Eddie Bergeron  and Michael Regan  yet - if not, can you touch base with them please and let us know here when they will be planning to send it to you?

stscijgbot-jp commented 1 year ago

Comment by Howard Bushouse on JIRA:

Anton Koekemoer No, have not received the code yet from either Eddie Bergeron or Michael Regan . Pinging here and via non-Jira channels.

stscijgbot-jp commented 1 year ago

Comment by Eddie Bergeron on JIRA:

Sorry for the delay, I'm working on it. Hopefully my cleanup and additional comments will make it easier for you to implement. Will aim to post it here by the end of the day tomorrow...ready or not.

stscijgbot-jp commented 1 year ago

Comment by Eddie Bergeron on JIRA:

Almost there. Just need a few more tests after the cleanup to make sure I didn't break anything. Will have it done today.

stscijgbot-jp commented 1 year ago

Comment by Eddie Bergeron on JIRA:

Ok, here's the main idl routine. There are some function calls. I'll post the other files, some test datasets and a little more description tomorrow. There's a brief algorithm overview at the top of the routine.

[^fix_miri_emi.pro]

stscijgbot-jp commented 11 months ago

Comment by Maria Pena-Guerrero on JIRA:

Started to work on this. The step name I am going with for now is "emicorr" is everyone OK with this? 

stscijgbot-jp commented 11 months ago

Comment by Anton Koekemoer on JIRA:

thanks, fine with me

stscijgbot-jp commented 11 months ago

Comment by Maria Pena-Guerrero on JIRA:

what are all the EXP_TYPE values this step will be applied to?

stscijgbot-jp commented 11 months ago

Comment by Howard Bushouse on JIRA:

The description mentions LRS slitless as having this problem, and I'm assuming Imaging mode would too (it has the most number of subarrays allowed), and perhaps subarrays used for coronagraphy as well?  So that would include MIR_IMAGE, MIR_LRS-SLITLESS, MIR_LYOT, and MIR_4QPM.  Eddie Bergeron Michael Regan can you confirm?

stscijgbot-jp commented 11 months ago

Comment by Michael Regan on JIRA:

In general, this could run for any MIRI detector in any mode. 390 Hz is only in subarrays but there is a significant effect at 10 Hz that is in all read out modes.

 

stscijgbot-jp commented 11 months ago

Comment by Eddie Bergeron on JIRA:

Ok, I've made a few mods to the fix_miri_emi.pro routine, fixing some bugs and adding a few features. The algorithm itself is unchanged. I've bundled up the main routine with all of the called subroutines and some example reference wave fits files into the attached tar file:

[^fix_miri_emi_lib.tar]

"tar xvf fix_miri_emi_lib.tar" will create a subdir called fix_miri_emi_lib with all the necessary routines in it. There are three main tools, and the rest are just dependencies. The example reference wave fits files are also in this subdir. The main tools to be concerned with are:

[^fix_miri_emi.pro]   ; this is it! (I've deleted the older version that was posted in this ticket).

[^miri_fft.pro]    ;  A utility tool for making a noise power spectrum of any miri exposure

[^miri_frametime.pro]    ; A tool for calculating the miri frametime of any fast or slow fullframe or subarray, based on Mike Ressler's frame timing doc

 

No need to code the latter two for the pipeline, but they are useful for other things as shown in the example calls in fix_miri_emi.pro. In particular, fix_miri_emi.pro needs some basic timing numbers for each subarray like rowclocks and frameclocks, however I've hardcoded those values into the case statements at the top of the main routine. Those should not change for the existing miri full/subroutines, but if new subarrays are defined you can use miri_frametime.pro to calculate the new values (or use Mike's own python routine for doing that - see doc reference in miri_frametime.pro).

 

The easiest way to set things up in idl is to just add the fix_miri_emi_lib dir to your idl path after starting idl. If you want to run this code and have not used idl before, just follow the steps here. You can install idl on your machine using the self service portal, and you will get a network license as long as you are on the staff network or vpn. Start idl and set the path:

 

IDL> !path='/path/to/where/you/put/fix_miri_emi_lib:'+!path

 

Be sure to put this directory FIRST in your path. Idl uses precedence, and I've modified a version of fits_write.pro to correct an issue with the default version in astronlib that was making the output fits files incompatible with pipeline input. If you get errors trying to use the output in the pipeline, check to make sure you have this order correct, or at least make sure you are using my version of fits_write.pro and not the astron or other library versions.

 

There are a set of calling examples at the top of fix_miri_emi.pro. Give those a try. I've also included an example of how to create a reference wave file from any given dataset.

 

Changes from my previous version:

 

Give it a try and let me know if you run into problems. Anyone else, feel free to try the idl routine on your favorite _uncal.fits  datasets and push them through the pipeline. In a following post to this ticket I'll include a few example plots and discussion about how to identify frequencies for correction and how to get them at the necessary precision to stay in phase across large datasets.

 

Regarding the reference files. As currently written, this code reads individual reference waves from separate fits files as an amplitude vector (only, evenly spaced in phase from 0-1.0) with the header keyword FREQ specifying the frequency. Since different full/subarrays will have different correction needs - only some subarrays need 390Hz correction, while all full/sub can benefit from a 10hz correction, and some subarrays have multiple noise components - I'd suggest a single reference file format, with either a table of variable length records for the wave vectors, or a multi-extension fits with an extension for each wave vector, and then keywords with each record/extension indicating which subarrays it should be applied to. Then the EMICORR step can recursively go though and apply each appropriate correction. New waves can be added by simply adding new records or extensions to the single reference file. If you feel it necessary, the phase at each amplitude could also be specified, but so far this seems to work with enough precision just assuming equal phase spacing in the reference wave vector. There is a reference_wave phase per bin calculation in the code ("pa_phase") but it is currently unused.

stscijgbot-jp commented 11 months ago

Comment by Maria Pena-Guerrero on JIRA:

thank you, I'll start working on this.

stscijgbot-jp commented 11 months ago

Comment by Maria Pena-Guerrero on JIRA:

I am having trouble locating Mike's python code to calculate the time frames, can you please add it in the ticket too, please?

stscijgbot-jp commented 11 months ago

Comment by Eddie Bergeron on JIRA:

The reference is: DFM478_fps_timing_fpga2_initial.pdf (MIRI DFM 478 - 04.02 "MIRI FPS Exposure Time Calculations", (SCE FPGA2), 28 October 2014, M. Ressler, MIRI Project Scientist, JPL. Here is the doc, python code in the text. I believe the equivalent numbers you need for "rowclocks" and "frameclocks" in fix_miri_emi.pro from Mike's code are roi_row_clocks and frame_clocks, but check these against what miri_frametime.pro returns. Should be the same, but if different let me know. My idl version is not a translation of his python, it was developed directly from the prescription in the doc.

Note that I've hardcoded some of the current flight constants in miri_frametime.pro that are required params in calls to Mike's code, so look for those params in miri_frametime.pro. Technically some of these could change in the future, but that's unlikely.

edit: Maria, I see that this doc is marked itar. I'll e-mail you with the location of this doc in the itar area on central store.

stscijgbot-jp commented 11 months ago

Comment by Eddie Bergeron on JIRA:

Those other "flight constant" params, like the main subarray definitions themselves (colstart, colstop, rowstart, rowstop), should be in a database somewhere. 

stscijgbot-jp commented 10 months ago

Comment by Maria Pena-Guerrero on JIRA:

I created the following dictionaries based on the docstring in the IDL script, to correct for the frequencies according to subarray. Eddie Bergeron and/or Michael Regan, can you please that the dictionary of frequencies to correct for according to subarray is correct or how should it be changed? Please look at the image frees2correct.png: 

!freqs2correct.png|thumbnail!

Thank you. 

 

stscijgbot-jp commented 10 months ago

Comment by Maria Pena-Guerrero on JIRA:

I have completed the implementation of the step. We now need to request CRDS to add a rule to look for a reference file for the emicorr step. For now, the resulting value can be set to 'N/A'. Howard Bushouse should I file the Jira request or should MIRI? 

stscijgbot-jp commented 10 months ago

Comment by Maria Pena-Guerrero on JIRA:

Another question Howard Bushouse, Michael Regan, Eddie Bergeron, is the plan to have 1 reference file with all frequencies to correct for OR 1 file per frequency?

(I am coding the file per frequency approach)

And, are we allowing the user to supply a reference file? (if so, as the code is right now, they would have to submit a file per frequency)

stscijgbot-jp commented 10 months ago

Comment by Howard Bushouse on JIRA:

For the reference file(s) I would think that you would at least bundle things to the level of having one ref file per subarray, which would contain the list of freqs to correct for that type of subarray. Then the ref file would use SUBARRAY as a selector in CRDS. Or you could even bundle everything up into a single ref file that contains a table (or equivalent) listing freqs for each subarray type. The cal step would then have to find the matching subarray entry in the ref file and read the list of associated freqs. The only downside to that approach is that if you want to change the list for one subarray you have to redeliver the whole ref file. Oh, and I would encourage the use of a ref file in ASDF format, rather than the clunky FITS BINTABLEs.

stscijgbot-jp commented 10 months ago

Comment by Maria Pena-Guerrero on JIRA:

Hmmm... Howard Bushouse, in thinking about it a little bit more, from the instrument's team it would be painful to do any changes to the frequencies because then all the files would have to be modified. So I think it makes more sense to have all frequencies in one file and choose from them according to subarray. This would make it easier for users to create their own reference file as well. I am using the fits format, should I switch to ASDF and abandon fits entirely?

stscijgbot-jp commented 10 months ago

Comment by Howard Bushouse on JIRA:

Yes, for this type of info, which is a list or table of entries, as opposed to something like an image, I would strongly advocate for a ref file in ASDF format. So much easier to work with than FITS BINTABLEs. There are examples of similar ASDF ref files already in CRDS.

stscijgbot-jp commented 9 months ago

Comment by Maria Pena-Guerrero on JIRA:

Misty Cracraft The emicorr step code is complete and ready for a full test but we first need you to please put in a request with the CRDS team to create a rule for this new step. The reference file is expected to be in ASDF format (I attached and example: jw01386007001_04101_00006_mirimage_emi_ref_waves.asdf), but for now the response value for the emicorr step in CRDS can be set to 'N/A', this will allow the step to run with an on-the-fly reference file creation or a user-supplied one.

Thank you.

stscijgbot-jp commented 9 months ago

Comment by Michael Regan on JIRA:

Where to I find the expected format of the ASDF reference file?

stscijgbot-jp commented 9 months ago

Comment by Maria Pena-Guerrero on JIRA:

Michael Regan There is an example ASDF reference file attached to this ticket, and on the CRDS ticket. The later might be better since it was modified to have all the expected values for selection, see https://jira.stsci.edu/browse/CRDS-745

stscijgbot-jp commented 9 months ago

Comment by Misty Cracraft on JIRA:

Maria Pena-Guerrero  Could you provide Mike with the script you used to create the file? Not all of us are adept at reading asdf formats and being able to replicate them. (speaking for myself here)

stscijgbot-jp commented 9 months ago

Comment by Michael Regan on JIRA:

Maria Pena-Guerrero I've tried to read the example file but I haven't been successful. When I run I get:

af = asdf.open(infile) ValueError: Can't handle '['jw01386007001_04101_00006_mirimage_emi_ref_waves.asdf']' as a file for mode 'r'

Do you know what I'm doing wrong? Thanks, Mike

stscijgbot-jp commented 9 months ago

Comment by Michael Regan on JIRA:

I found the YAML schema in the PR. Now, if I could just figure out why I can't read the example file I should be able to make progress.

 

hbushouse commented 9 months ago

If you’ve got the stdatamodels PR branch installed that contains the new data model definition, you create the ref file by just creating a model of that type, populating the various attributes/arrays in it, and then “model.save()” does the writing of the ASDF file for you. You don’t have to create the ASDF file by hand. Same thing for reading. Just do a “datamodels.EmiCorr(“my_ref_file.asdf”)” or whatever the model name is and it should load it into a model. Don’t read directly with “asdf.open()”.

From: stscijgbot-jp @.> Reply-To: spacetelescope/jwst @.> Date: Tuesday, November 21, 2023 at 10:28 AM To: spacetelescope/jwst @.> Cc: Subscribed @.> Subject: Re: [spacetelescope/jwst] MIRI Subarrays 390 Hz EMI (Issue #7729)

Comment by Michael Reganhttps://jira.stsci.edu/secure/ViewProfile.jspa?name=mregan on JIRAhttps://jira.stsci.edu/browse/JP-3248?focusedCommentId=672707#comment-672707:

I found the YAML schema in the PR. Now, if I could just figure out why I can't read the example file I should be able to make progress.

— Reply to this email directly, view it on GitHubhttps://urldefense.com/v3/__https:/github.com/spacetelescope/jwst/issues/7729*issuecomment-1821146157__;Iw!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!zuQHPfbj_3z022V2_0n9QsX9oQ1jlwKCfmI_BZb9W2QtlGsz5zT-zB_FCyjqdnP7HcxocyWpzZDIpngK35-r2NAP5Q$, or unsubscribehttps://urldefense.com/v3/__https:/github.com/notifications/unsubscribe-auth/ADCDVNGE2D5OLZGL5GTKSRLYFTCBRAVCNFSM6AAAAAA2KWAMLGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRRGE2DMMJVG4__;!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!zuQHPfbj_3z022V2_0n9QsX9oQ1jlwKCfmI_BZb9W2QtlGsz5zT-zB_FCyjqdnP7HcxocyWpzZDIpngK35_8MzlQng$. You are receiving this because you are subscribed to this thread.Message ID: @.***>

stscijgbot-jp commented 9 months ago

Comment by Michael Regan on JIRA:

But why can't I read the sample file? If I can't read asdf files, I won't be able to validate the ref file.

stscijgbot-jp commented 9 months ago

Comment by Maria Pena-Guerrero on JIRA:

Michael Regan I just did this and it opened fine:

In [1]: import asdf

In [2]: f='/Users/pena/Downloads/jw01386007001_04101_00006_mirimage_emi_ref_waves.asdf'

In [3]: af=asdf.open(f)

In [4]: asdf.{}version{} Out[4]: '2.15.1'

In [5]: af.info() root (AsdfObject) ├─asdf_library (Software) │ ├─author (str): The ASDF Developers │ ├─homepage (str): http://github.com/asdf-format/asdf │ ├─name (str): asdf │ └─version (str): 2.15.1 ├─history (dict) │ └─extensions (list) │   └─[0] (ExtensionMetadata) │     ├─extension_class (str): asdf.extension.BuiltinExtension │     └─software (Software) ... ├─Hz10 (NDArrayType): shape=(128,), dtype=float64 ├─Hz164 (NDArrayType): shape=(128,), dtype=float64 ├─Hz218a (NDArrayType): shape=(128,), dtype=float64 ├─Hz218b (NDArrayType): shape=(128,), dtype=float64 ├─Hz218c (NDArrayType): shape=(128,), dtype=float64 ├─Hz390 (NDArrayType): shape=(128,), dtype=float64 └─meta (dict)   ├─date (str): 2023-10-30T15:30:21.286   ├─filename (str): jw01386007001_04101_00006_mirimage_emi_ref_waves.asdf   ├─instrument (dict)   ├─model_type (str): EmiModel   └─telescope (str): JWST Some nodes not shown.

In [6]: 

stscijgbot-jp commented 9 months ago

Comment by Maria Pena-Guerrero on JIRA:

Misty Cracraft I didn't use a script to create the ASDF file, I used the corresponding datamodel to save the dictionary I created with the pipeline step, and the datamodel took care of everything else. I used an ASDF template to tell the datamodel schema both that the file will be in ASDF format, and how to store the data. The schema located at: https://github.com/penaguerrero/stdatamodels/blob/emicorr/src/stdatamodels/jwst/datamodels/schemas/emi.schema.yaml

stscijgbot-jp commented 9 months ago

Comment by Michael Regan on JIRA:

Well, as a last resort I replaced the variable containing the file name with the text string and it worked. Who knew?

stscijgbot-jp commented 9 months ago

Comment by Maria Pena-Guerrero on JIRA:

 Eddie Bergeron and/or Michael Regan, can you please that the dictionary of frequencies to correct for according to subarray is correct or how should it be changed? Please look at the image frees2correct.png

I created the following dictionaries based on the docstring in the IDL script, to correct for the frequencies according to subarray: 

!freqs2correct.png|thumbnail!

I need to know if this information is correct in order to run tests further tests.

Thank you. 

stscijgbot-jp commented 9 months ago

Comment by Eddie Bergeron on JIRA:

Hey Maria, I just sent Mike 4 preliminary reference waveforms to put in the ASDF reference file:

390hz_reference_wave_sub128.fits - 390 hz for SUB128 390hz_reference_wave.fits -390 hz for all the other affected subarrays (SLITLESSPRISM, MASKLYOT, SUB64, MASK1140, MASK1065, MASK1550) 10hz_reference_wave.fits - 10 hz for all fastmode fullframe and subarrays (including the BRIGHTSKY and SUB256 subarrays which don't have 390) mirifulong_slowmode_10hz_reference_wave.fits - 10 hz for all slowmode fullframe

For now, please remove all the 218* and 164 frequencies from the dictionary and add two new entries - one for the "SUB128-specific 390hz" and one for "slowmode 10hz."

It's likely there will be 3 separate slowmode 10 hz models, one for each detector. This is a fairly new development. I noticed significant differences in the shapes and amplitudes of 10hz across the 3 detectors and between fast and slow, likely due to the thermal dissipation being different in each of their PID control laws (generally higher backgrounds on the MRS and LRS than the imager).

You probably haven't coded for different waveforms per detector but it looks like that might be necessary. For now I've just provided Mike with one 10z each for fast and slow and this should be good enough for some testing. If you add detector specific options later we can test against new reference waves then. Here's a plot showing the general differences I'm seeing between detectors: !miri_10hz_fast_vs_slow.png|thumbnail!

stscijgbot-jp commented 9 months ago

Comment by Eddie Bergeron on JIRA:

The MIRI team will almost certainly want to add additional waves for other frequencies (e.g. 218, 164hz), but I don't have time at the moment to work through those. The 164 and 218 appear in different subarrays (the 164 is in MASKLYOT, but the 218 appears in the other MASK* subarrays). The good thing is they are much lower amplitude than the 390, but still significant enough to see visually in the data. You may notice them left behind after the 390 subtraction, but they are at a higher frequency. Look for them while testing to see if you can pick them out. I can post a few more examples tomorrow to demonstrate. There's a masklyot example at the top of my fix_miri_emi.pro routine with the 164 hz subtraction in addition to 390 and 10hz.

stscijgbot-jp commented 9 months ago

Comment by Rosa Diaz on JIRA:

Maria Pena-Guerrero, after discussing with Michael Regan and Eddie Bergeron that they need to test the subarray frequencies before deciding on what should be applied per subarray, so having them in the code is not going to work. Setting the amplitude to zero for the ones they don't want to apply might break things. We have until Dec 5th to submit reference files that require code changes. It is possible to change the code to read all this information from one reference file? The important thing would be how the format of this reference files should be. Ideally one datablock should have frequencies and their arrays while the other should have the subarray and frequencies to apply. Also, Eddie mentions that we we might need select by detector too.

stscijgbot-jp commented 9 months ago

Comment by Maria Pena-Guerrero on JIRA:

Rosa Diaz Eddie Bergeron Michael Regan Regan Let me check my understanding of your comment:

Add a "detector" key to the dictionary

Add dictionary key 390Hz_sub128 only for this subarray

Correct with regular 390Hz for subarrays: SLITLESSPRISM, MASKLYOT, SUB64, MASK1140, MASK1065, MASK1550

Correct for 10Hz for fastmode fullframe and all subarrays (including the BRIGHTSKY and SUB256 subarrays which don't have 390)

Remove 218* and 164 frequencies from the dictionary

Add 3 frequencies for 10Hz_slowmode_det and use to correct for all slowmode fullframe

 

If we do this then I think a single reference file works, I just need to add these new frequency names to the reference file. I can create a new mock reference file and post it here.

Is this the way to go?

stscijgbot-jp commented 9 months ago

Comment by Rosa Diaz on JIRA:

Maria Pena-Guerrero will the subarray selection for the wavelength will still be in the code or is something that is in the reference file? What would happen if after testing Mike and Eddie decide they want to add another frequency to one of the subarrays. Can it be done via reference files?

stscijgbot-jp commented 9 months ago

Comment by Eddie Bergeron on JIRA:

Hey Maria, I think the 6 items you listed above are a good starting point for a reference file, although it seems like Mike can modify the reference file as needed when we add the reference wave data to it.

However we are a little confused about the dictionaries and where they are located. We'd like to see all of the "refwave, frequency, subarray, and detector" info in the reference file itself, with no mention of this anywhere else. The code itself should be completely generalized to get what it needs from the reference file (or optional parameter inputs, e.g. for the "self correction" option). So if MIRI decides to add a new frequency, all they should need to do is modify the reference file(s) and that's it.

As to whether to use separate reference files per detector vs. using dictionary definitions within a single reference file to select refwaves by detector, that's up to you guys to decide. 3 separate reference files vs. one is no biggie, even if some of the refwaves are identical across all of them, but it seems like it should be possible to put them all into one reference file with a set of "detector+subarray" tags on each reference wave. Whatever you think is easiest.

We just want to be absolutely sure there is no need to modify the code or a separate definitions table when a new refwave is added/modified/removed. The only thing that should be needed in that case is a new reference file.

stscijgbot-jp commented 9 months ago

Comment by Maria Pena-Guerrero on JIRA:

The reference file only contains the phase amplitudes for the frequencies with names: Hz390, Hz218a, Hz218b, Hz218c, Hz164, and Hz10. All these are expected to be a 1-D float array. This is specified in the datamodels repository.

Now the pipeline jwst repository, in the emicorr step, contains the 2 key dictionaries for the step: known_emi_freqs dictionary (contains the name of the frequency - which needs to match what is in the reference file), and the subarray_cases dictionary (contains the subarray names along with the corresponding rowclocks, frameclocks, and freqs - this last key is a list containing the frequencies to correct for the subarray). I will add the detector key to this last dictionary.

So if in the future a new frequency needs to be added, we will need to submit 2 pull requests, one for the datamodels repository to add it to the emi schema, and the other pull request for the jwst repo so that the new frequency is added to both dictionaries, known_emi_freqs and subarray_cases.

the only way to do what you are asking for, i.e. to not touch the pipeline code and only add a new reference file would for you to create a reference file per subarray. This means a modification in the datamodels repo (in order to create a generic reference file datamodel), the jwst pipeline repo (to remove the dictionaries mentioned above and simply correct for everything in the reference file), and in the CRDS rules (we would need to add the subarray and detector as search parameters for the reference file). Is this the way you want to go?

 

stscijgbot-jp commented 9 months ago

Comment by Eddie Bergeron on JIRA:

Why does a known_emi_freqs_dictionary need to be kept somewhere other than the reference file itself? The "known emi frequencies" are by-definition the ones in the reference file. The code can look at the asdf reference file to get those. The subarray definition info (rowclocks, frameclocks) is fixed for a given pre-defined MIRI subarray so it's fine to pull that from a different place.

The thing is, if an end-user wanted to use their own reference wave amplitude vector or even add a different frequency there would be no way for them to do that because of this pairing with a dictionary they don't have access to. Ideally any user (or the MIRI team) could simply make their own version of the asdf reference file to add any frequency they want and the pipeline code would just get that from the reference file and use it.

stscijgbot-jp commented 9 months ago

Comment by Rosa Diaz on JIRA:

Maria Pena-Guerrero  I understand the sending two pull requests to change the frequencies is very easy, the problem is that these changes can only be pushed to the operational pipeline via builds (every 3 months or more). And if a problem is identified in between I am not sure if we can even turn off this correction. That is the reason for which we believe the assignment of the frequencies to subarrays or detectors should be in a reference file and not in a dictionary in the step itself.