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

Deal with header keywords #305

Open
teutoburg opened this issue Dec 7, 2023 · 2 comments
Open

Deal with header keywords #305

teutoburg opened this issue Dec 7, 2023 · 2 comments

Comments

@teutoburg
Copy link
Contributor

          We (HB, JK, FH, KL) discussed the headers in [https://github.com/AstarVienna/ScopeSim/blob/f9e10426d283d3188a9094782b9e5a5b50f7f8ff/scopesim/optics/optical_train.py#L334](`write_header()`] in `optical_train.py`.

Ultimately it would perhaps be best to put all headers in FitsKeywords Effects, which is then included by default. But at least for now it is easier to keep write_header(), clean it up so it contains only headers we always want (mostly WCS), and call it always before calling the FitsKeyword effects.

The plan:

  • Keep the WCS headers, but copy those from the image plane if possible, otherwise use astropy to construct them. The logic about the SpectralTraceList WCS should at some point be moved to earlier in the simulation so that no special logic should be necessary here, but for now we can keep it.
  • Anything ScopeSim specific, e.g. filenames and things we cannot know with the real data (like dark current), should be removed. The existing fits header effects should cover those, e.g. SimulationConfigFitsKeywords and EffectsMetaKeywords. It is up to the user / instrument to include those, but maybe we can make them a default.
  • Source related keyword will be replaced by SourceDescriptionFitsKeywords.
  • For the other keywords we will decide whether they are generic enough to be included, or instrument specific, and should be moved to the IRDB.

I realize now that we should check whether the existing keyword effects are suitable enough as a default. As in, we don't want to break too many workflows of people; maybe for some information, like filter selection, we should do something sensible as default.

We decided we can merge this PR without doing all of the above, because we have a plan, and merging this PR will only affect METIS.

Originally posted by @hugobuddel in AstarVienna/irdb#146 (comment)

@teutoburg
Copy link
Contributor Author

I'll quickly summarize some of my findings about the WCS in this context:

  • The FOVs in OpticalTrain.fov_manager.fovs have both detector and sky WCS in their headers.
  • After FOV-effects are applied, the FOVs get added to OpticalTrain.image_planes using only the detector WCS (wcs_suffix="D").
  • The FOVs are subsequently no longer considered, so the sky WCS effectively gets lost in the previous step.

Maybe it would be possible to also store the sky WCS from the FOVs when they're added to the ImagePlane, then they would be available in the header afterwards and could be written to the output.

@teutoburg
Copy link
Contributor Author

Adding to that so it's written down somewhere:

The FOVs' WCSs cannot be used directly for the output WCS, as they generally don't match the whole detector's FOV...

Need to find a smarter solution

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

No branches or pull requests

1 participant