-
Notifications
You must be signed in to change notification settings - Fork 108
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
SLITLESS FLAT #1820
SLITLESS FLAT #1820
Conversation
Tests pass (except the usual setup gui unrelated failures)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! I have lots of minor comments and two major ones. The major ones are:
- Given that you've already done the work, I'm not sure it's worth going back on it, but there's a structural comment I wanted to bring up. To me, it would seem cleaner to have a separate script that builds the slitless pixel flats instead of trying to integrate the creation of these flats within the overall workflow of
run_pypeit
. There are some gymnastics you have to go through to get the latter to work that I think would be better separated out into more dedicated scripts. A dedicated script would also more cleanly separate the interaction with the cache. I.e., we could change to a mode whererun_pypeit
is only using slitless flats that can either be downloaded or installed in the local cache. Is a separate script something you thought through? - I think we should create a new
DataContainer
subclass for these slitless flats.
The last thing I wanted to highlight is that I think we should be making more use of the get_file_path
function for interaction with the cache. I.e., there are a few places in the code where you effectively repeat code that is the purpose of that function. The trouble is that you will often need to wrap it in a try/except block. If this becomes burdensome, we could alter that function to avoid that.
Let me know what you think!
|
||
""" | ||
|
||
return np.arange(len(fitstbl)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand the general application of this function. Given that it's only used for slitless flats right now, do we need a general function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually spectrograph dependent, this is why it's here. It is currently only used for keck_hires
, but I tried to make it generic enough so that it could be used for other purposes (still spectrograph-specific). I have added more in the docstring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and few comments to chew on. thx!
pypeit/calibrations.py
Outdated
detname_list.append(self.spectrograph.get_det_name(_det)) | ||
|
||
if len(detname_list) > 0: | ||
# generate the slitless pixel flat file name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should probably be its own stand-alone-method somewhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I addressed all the minor-ish comments.
I have left:
- New datamodel for SlitlessFlats
- The cache stuff
|
||
""" | ||
|
||
return np.arange(len(fitstbl)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually spectrograph dependent, this is why it's here. It is currently only used for keck_hires
, but I tried to make it generic enough so that it could be used for other purposes (still spectrograph-specific). I have added more in the docstring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kbwestfall @profxj I am done addressing the comments. Please take a look.
@kbwestfall I was not able to create a Datacontainer for SlitlessFlat (too complicated), but I re-organized the code and created a SlitlessFlat class that holds the code for crating the slitless flats.
I was also able to make adjustments to the cache for this PR, by adding a small change to get_file_path
. Let me know what you think.
@kbwestfall @profxj I was finally able to run the tests (Google Drive is giving me hard time). ALL GOOD!
NOTE:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the slow response on this one! This looks good and I'm approving. Two things for future PRs:
- I'd like to help see if I can get the SlitlessFlat class to subclass from DataContainer.
- I worry a little about adding ~200MB in fits images, but these should not change often meaning that they shouldn't blow up the size of the repo. We should keep track of this and possibly remove them (and clean-up the commit history) if the repo gets too big.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
excellent! great to have this
This a large work aimed at introducing the creation of slitless flat in the reduction workflow. Below is the summary of changes.
slitless pixelflat are generated during the reduction and saved in pypeit/data/static_calib and can be archived for future uses
added a script that show the archived pixelflat in ginga
archived 3 new keck_hires slitless pixflat and 1 new keck_lris slitless pixelflat
when multiple images are combined during the reduction, now there is the possibility (including a new parameter) to scale the images to the mean values
improved frame typing and configuration setup for keck_hires and keck_lris
fix a bug with argsort
resolved the non_linear_counts TODOs
STILL MISSING: DocumentationDev Suite PR: pypeit/PypeIt-development-suite#324