-
Notifications
You must be signed in to change notification settings - Fork 105
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
Spatial flexure options #1855
base: develop
Are you sure you want to change the base?
Spatial flexure options #1855
Conversation
# Conflicts: # pypeit/spectrographs/gemini_gmos.py
# Conflicts: # doc/releases/1.16.1dev.rst # pypeit/flatfield.py
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.
Thanks @debora-pe - I've responded to all comments, updated and rebuilt the docs. There's two points that still need to be considered:
- What to do with the metadata
MASKED_VALUE
. Are there better options? - I'm not quite sure we're handling the
manual_flexure
correct, so would be great to get some clarity on this.
I'll re-run the dev suite to make sure I haven't broken anything obvious!
pypeit/pypeit.py
Outdated
if (manual_flexure or manual_flexure == 0) and not (np.issubdtype(self.fitstbl[frames[0]]["shift"], np.integer)): | ||
msgs.info(f'Implementing manual flexure of {manual_flexure}') | ||
spat_flexure = np.float64(manual_flexure) | ||
if (manual_flexure != self.fitstbl.MASKED_VALUE) and np.issubdtype(self.fitstbl[frames[0]]["shift"], np.integer): |
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 quite sure about this. My understanding of the previous code is:
(manual_flexure or manual_flexure == 0)
if manual_flexure
is anything other than None
and not (np.issubdtype(self.fitstbl[frames[0]]["shift"], np.integer))
and it's set to something other than an integer.
The first line seems like this is always true, since the previous default in metadata.py
would set shift=0
as the default (unset) value. I think the MASKED_VALUE
clears this up. Then, the second line has me more confused why this is there... For example, the simple examples:
In [2]: not np.issubdtype(np.zeros(2, dtype=float).dtype, np.integer)
Out[2]: True
In [3]: not np.issubdtype(np.zeros(2, dtype=int).dtype, np.integer)
Out[3]: False
So, I think this if
statement is only being triggered when the shift
value is a float. I don't really understand why this is, but I assumed folks would input a integer pixel value (hence my change). Perhaps this should just be:
if manual_flexure != self.fitstbl.MASKED_VALUE:
pypeit/metadata.py
Outdated
@@ -1576,7 +1580,7 @@ def set_user_added_columns(self): | |||
if 'manual' not in self.keys(): | |||
self['manual'] = '' | |||
if 'shift' not in self.keys(): | |||
self['shift'] = 0 | |||
self['shift'] = self.MASKED_VALUE |
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.
That's correct - if someone sets manual_shift=0
, then the manual shift wouldn't be applied, because the manual_shift
is set to the (hard-coded) masked value.
I am also not a huge fan, but I don't have a better solution. Although, I thought the -9999
wouldn't appear in the .pypeit
file, it's only something that's hidden in the code. Am I mistaken about this?
Co-authored-by: Debora Pelliccia <[email protected]>
Spatial flexure reorganisation
# Conflicts: # doc/releases/1.17.1dev.rst
# Conflicts: # doc/releases/1.17.1dev.rst # presentations/py/users.py # pypeit/wavetilts.py
This PR reworks a lot of the code that uses spatial flexure. The main change here is that instead of a single value, the spatial flexure is now stored for each slit edge. I haven't yet implemented the edge-by-edge spatial flexure adjustment. These updates are just to ensure that the code base is unaffected by the change from a single floating point value to a 2D array.
There is a matching PR in the dev-suite. I'm running tests now...