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

Add a recipe of estimateNoise #1

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Conversation

leejjoon
Copy link

The PR adds a new recipe of estimateNoise, which will work with flat off images. While the recipe is more-or-less for QA purpose, it is adequate to test various aspects of the DRAGONS recipe system. It also includes updated AstroData class for IGRINS. For now, the recipe expects idata from IGRINS, i.e., I assume there are separate files for H and K detectors. I understand that IGRINS2 will have both H and K in a single file, and it should be straight forward to adopt it.

You can install it via 'pip install' command. Currently the 'pyproject.toml' file does not contain any dependency for dragons. So, you need to manually install dragons before installing igrinsdr.

> pip install .

Once installed, you should be able to do

> dataselect --tags FLAT test/20220301/SDCH*.fits -o list_of_flat.txt --adpkg=igrins_instruments
> reduce @list_of_flat.txt --drpkg=igrinsdr --adpkg=igrins_instruments

@KathleenLabrie
Copy link
Contributor

I'm reviewing this today. The first thing that will need to be corrected is the use of the src directory. Please restore the structure that was there before. There is a reason for it. However, I probably should ask, is there a strong technical constraint on your side for this change?

Also related to the structure, we keep the tests "with the code". As such, the 'astrodata' test should be moved under igrins_instruments/tests/. Thanks for thinking about tests right away.

Where can I get the 20220301/SDCH*.fits files?

I'll be digging deeper into the code today. It looks like a great start.

@KathleenLabrie
Copy link
Contributor

igrins_instruments/igrins/init.py

Is there a reason why there are two AstroDataIGRINS in the all list? Is the igrins in gemini_instruments causing trouble? Looking at adclass, should they be AstroDataIGRINS1 and AstroDataIGRINS2?

On line 10, the “fox” should be replaced with “IGRINS”. My mistake when I set up the template.

--

igrins_instruments/igrins/adclass.py

Looks good. We’ll use that for development. There might be a way to combine the AstroDataIGRINS1 and AstroDataIGRINS2 classes into one class eventually but your implementation is easier to handle and to understand for now.

The data_label descriptor for IGRINS-2 will have to use the DATALAB keyword eventually because IGRINS-2 will be a facility instrument and will have all the standard Gemini keyword values. At some point, the IGRINS-2 simulated or test data will have to also simulate the standard Gemini keywords instead of the IGRINS makeshift one.

--

igrinsdr/igrins/igrins_pipeline/utils/image_combine.py

The use of stsci.image will need to be replaced. STScI seems to have abandoned support. The package is only available for Python 3.7 and lower. DRAGONS v3.1+ will require Python 3.9+. (Also, note that it would be advisable to do the IGRINS-2 development with Python 3.9.)

--

igrinsdr/igrins/igrins_pipeline/procedures/*

I just checked the imports not the code. Looks okay.

--

igrinsdr/igrins/lookups/timestamp_keywords.py

Each primitive that you write needs a timestamp keyword.

--

igrinsdr/igrins/procedures

I’m curious, how is the “procedures” directory here different from the “igrins_pipeline/procedures”?

--

igrinsdr/igrins/recipes/sq

The recipes are not quite right and we will need to discuss this. More below.

--

igrinsdr/igrins/recipes/sq/recipes_DARK.py

The makeProcessedDark has the right idea, but I think that the noise is not handled and stored in the “DRAGONS way”. We will need to find a better name for selectLevel3Removed as it is really cryptic. (We do spend quite a lot of time discussing how we name things. It helps make the whole system more uniform across instruments.)

--

igrinsdr/igrins/recipes/sq/recipes_FLAT.py

The estimateNoise recipe should probably not exist. The output of a recipe is supposed to be a science output or a recognized calibration that can be associated with the calibration manager. That recipe produces a loose file that can only be retrieved by adding it manually to a subsequent call.

This is in a FLAT recipe library, which means that the inputs are FLATs, yet it calls stackDarks(). Does the recipe require darks? I’m not sure that you really need to use streams here. The setSuffix primitive should not be necessary. There is very little DRAGONS in that recipe, it will need quite a bit of rework.

I will need to know more about what this recipe is producing and from what type of inputs. I suspect that these calculations need to be associated with a makeProcessedFlat recipes, and the results probably attached to the output processed flat.

I had started making DRAGONS-compatible flow charts for IGRINS. We probably should sit down together and look over them and see how to make it all fit the DRAGONS way.

--

igrinsdr/igrins/parameters_igrins.py

All primitives must have a suffix parameter. You cannot just pass.

--

igrinsdr/igrins/primitives_igrins.py

I would like to discuss the primitives that you have created. Some of them are probably not recommended or required. (eg. setSuffix) I believe that selectFrame can be replaced with a DRAGONS core primitive, selectFromInputs. That would require the inputs to be selectable through an astrodata tag. What are those FRMTYPE data? Flats, darks, what are the FRMTYPE data? BTW, we try to avoid accessing the headers directly in primitives. We use astrodata descriptors or tags.

streamPatternCorrected access the data as hdu_list. That is never done in DRAGONS, the data is accessed via the astrodata object. I see that you have to split it up to use an existing function, I understand why you did it. However, wouldn’t it be easier to just convert the function to use astrodata? You wouldn’t have to create copies of the pixels (memory intensive) and have to reconstruct the astrodata afterwards. For example, pass the ad objects to the function and in the function, access ad[x].data to get the numpy array. The work will be done in place, saving memory.

Why are you using stream in streamPatternCorrected. Streams can be messy, we use them only when absolutely necessary.

--

General comment.

I understand that the primary objective is to get something to work in DRAGONS with as little effort as possible, and some of the comments above do not necessarily require immediate action, they could cleaned up later. However, I think that we should sort out the recipes to make them DRAGONS-like. I’m more concerned about the recipes and the products (like where tables and solutions are stored) then the internal of the primitives.

I propose that we meet to work together on DRAGONS flowcharts. Once those are in place, I think that you will have no problems fitting the igrins algorithms into primitives. What you have accomplished already with no support from me is quite impressive.

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

Successfully merging this pull request may close these issues.

2 participants