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

Concept of longslit workflow as an example of specreduce block structure #71

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

eteq
Copy link
Member

@eteq eteq commented Oct 28, 2021

This PR adds a "concept" file that describes a modular workflow for how we can build specutils-compliant workflows that we can use as interface definitions.

Note that this is based heavily on the following diagram from the Astropy Coordination Meeting 2019:
image

This is currently for discussion, so I'm opening it as a draft, but it could get merged along side the concept notebooks if we settle on it with some revisions as a guide for future development



@dataclass
class CCDProc:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to duplicate the above?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, that's an obvious duplicate. i also think that this is best left to the ccdproc package or whatever a pipeline does to calibrate out detector-level level effects. spectroscopy-specific steps should start with a CCDImage and go from there.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops yes that's a mistake. But to clarify, I intended exactly what @tepickering said here: this should only wrap ccdproc functionality at best, and might literally just be ccdproc calls

@tepickering
Copy link
Contributor

i'll note that pypeit and perhaps other pipelines treat longslit data as multislit data with just one slit. i think it's better to design around the assumption that the 2D inputs can contain N spectra and work down from there than to cobble up from the N=1 longslit case. lots of different cases flow much more easily from the N spectra assumption: multislit, multifiber, echelle, ifu. so the wlsoln box is really a build wcs box where the dispersion axis comes from arcs and the spatial parts come from headers/metadata.

@eteq
Copy link
Member Author

eteq commented Nov 3, 2021

Hmm I mildly disagree that treating it as "the one slit case" makes sense. I think a lot of awkwardness in Python's past and pypeit itself stem from an unwillingness to think about "a spectrum" as an important element.

That said, I see your point that having the plan allow miltislit at the outset is easier. So this is really a semantic or maybe minor syntactic distinction. I think that could be pretty straightforwardly adapted from this API

@tepickering
Copy link
Contributor

i think we're mostly on the same page if there's an extra box for find spectra that generates what you call SpecCollection. for the longslit case, find_spectra() returns the whole detector and SpecCollection has length 1.

@jehturner
Copy link
Member

I agree that it makes sense to think of a long slit as a special case of MOS that just has 1 slit, though DRAGONS does not yet have such a concept, since we had to get long slit working ASAP and it's easier to deal with that first. I don't really understand @eteq's point though.

In DRAGONS we only have the equivalent of AutoIdentify1D, along with a step that calibrates arc line distortions in 2D pixel space (ie. x, y -> x', y, where x` is some reference row). There's no need to match a line list N times for N rows (modulo the question of different rows having slightly different wavelength ranges); it's easier & more robust to do it accurately once and then chain that calibration after a correction for wavelength distortion, with gWCS. Otherwise you can end up with, say, 10 out of 1000 rows that have a completely different solution from the others, as in IRAF, and a lot of interaction to fix them. Moreover, if automated line matching fails entirely, the distortion calibration can still succeed using unidentified lines, allowing steps like sky subtraction (which only needs the spectra to be straight) to proceed in an automatic pipeline. So I would be a bit careful about baking in assumptions about how this looks that are too fixed.

I vaguely recall some discussion about SpectrumCollection. Since Erik's diagram is for long slit, I assume that represents a different solution for each row, which is one way of looking at it, though @tepickering seems to be interpreting it as 1 solution per slit, which I would have thought makes sense. But even for MOS & IFU, it could make sense to calibrate distortions and possibly a wavelength solution over all the slits/apertures and just leave the zero point (also scale??) as a free parameter per slit. That may require some changes to modeling, to allow feeding one model's spatial co-ordinate into another's model set axis and vice versa (Nadia & I came up with a possible hack for an IFU case a few years ago, but I'll have to remind myself of the details).

We should bear in mind that the end result is not always extracted to 1D (but I don't think you intended to assume that).

@pllim
Copy link
Member

pllim commented Nov 29, 2021

@pllim
Copy link
Member

pllim commented Dec 2, 2021

@jradavenport
Copy link
Contributor

indeed, I've updated my workflow for https://github.com/jradavenport/kosmos, but I don't think it needs to be ingested here yet (unless we want to propagate other changes to methods I've made )

@duytnguyendtn
Copy link
Contributor

Wavelength dependent sky subtraction will be a step done before the actual extraction (i.e. the extraction methods assume both global and sky (wavelength-dependent) subtraction has already been completed on the incoming data). @eteq did I get this right?

@eteq
Copy link
Member Author

eteq commented Dec 8, 2021

Out-of-band it was pointed ouit this is missing a sky-subtraction step. I had mentally lumped this in with the extraction step, but it should be a separate step given that there's lots of different ways to do that, so should split BoxcarExtract into BoxcarExtract and BasicSkySubtract or something (with the background_width and background_fit parameters moved to BasicSkySubtract)

@eteq
Copy link
Member Author

eteq commented Dec 8, 2021

Oh, hah, what @duytnguyendtn said 😉 . I wrote the above before seeing #71 (comment)

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.

7 participants