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

Spectral tracing discussion #83

Closed
ojustino opened this issue Feb 23, 2022 · 14 comments
Closed

Spectral tracing discussion #83

ojustino opened this issue Feb 23, 2022 · 14 comments

Comments

@ojustino
Copy link
Contributor

This issue is meant as a forum to discuss the new tracing classes in specreduce/tracing.py since the PR that introduced them has already been merged.

I'll start by saying all of Trace, FlatTrace, and ArrayTrace work smoothly for me.

Here are some avenues of further discussion and possible changes to consider:

  • We don't currently have a way to calculate a trace based on an image. I support creating one, but is it a priority? If so, I think @jradavenport had said that we were OK to port code from the kosmos trace() function into specreduce.
  • As @ibusko mentioned here, it would be nice to validate that the trace position is greater than 1 and less than the cross-dispersion axis' length as the Trace object is being created instead of at the beginning of the extraction process.
  • In response to @tepickering's comment here, I support including dispersion/cross-dispersion axis information as attributes of Trace objects instead of as arguments of an extraction class' __call__() method. If we want to allow for more interchangeability, perhaps they could be set as arguments of Trace objects. If not, we could hard-code our assumption that axis 1 is dispersion and axis 0 is cross-dispersion into Trace. Either way, extraction classes would get that information for free since they must take a Trace object as an argument.
  • Should all Trace objects contain an image attribute? ArrayTrace and FlatTrace don't need an image, but future Trace implementations may. Including the image in the Trace object would also decrease the number of arguments needed for extraction objects. However, @ibusko brought up a point that you may not always want to perform an extraction on the exact image from which you created a trace.
@kecnry
Copy link
Member

kecnry commented Feb 23, 2022

Should all Trace objects contain an image attribute?

My current thinking is to not take the image unless it is required for the trace. If we want to implement detecting a trace from an image, I'd propose implementing trace = ArrayTrace.from_image(image) instead. This then makes it clear to the user, I think, when the image is actually being used and when it is not.

@ibusko
Copy link
Contributor

ibusko commented Feb 23, 2022

Second bullet above: it is OK unless we want to enable that a trace obtained with one image can be used on another. Then the extraction code itself will have to check for consistency.

4th bullet: I agree with it, and also with @kecnry 's proposal: if the trace class needs an image, we supply it within a specific method call sequence.

@tepickering
Copy link
Contributor

object finding and calculating traces based on detected objects is stuff i am actively working on, but not quite ready for a PR. i agree with adding an attribute to Trace objects to define which axis is being traced along. that will commonly be the dispersion axis, but not always! the signal finding/trace fitting tools could be used to trace emission lines along a spatial axis, for example.

i don't think Trace objects should necessarily have image attributes because there are a number of ways traces can be calculated. for multi-slit/multi-object data the traces are often determined from flat-field images. in other cases they're predefined by prior calibration work or theoretical modeling of a spectrograph's optics.

@jehturner
Copy link
Member

I should just mention FYI that my colleagues have been doing a lot of work on improving the detection & tracing of object spectra in DRAGONS recently and I believe it is working pretty well. They have been through various algorithms using wavelets etc. before finally settling on something.

@tepickering
Copy link
Contributor

ah, good to know. i have been looking at the DRAGONS code and basing what i want to do off of that. i'll update my local check-out and see what's changed in the past few weeks...

@jehturner
Copy link
Member

jehturner commented Feb 23, 2022

It looks like the latest work is in enh/aperture_tidy. You might want to wait for it to be merged before basing anything on it (I think that will probably happen soon).

@tepickering
Copy link
Contributor

cool! i'll take a peek at it at least.

@jradavenport
Copy link
Contributor

Just to chime in and say: totally fine to use the simple trace algorithm from KOSMOS. One bit of feedback we've had from early users is the strong desire for simple interactive trace/object selection... this is yet another GUI to consider (and something we've avoided doing in many places within specreduce). I don't think we need to provide such a GUI, but just to keep it in mind that there's an "identify" or first-guess step we should be exposing as something a GUI can plug into

@jehturner
Copy link
Member

jehturner commented Feb 24, 2022

I should perhaps note that DRAGONS does have a fairly well-developed Web GUI for findApertures (and several other reduction steps) -- eg. see https://github.com/GeminiDRSoftware/DRAGONS/blob/master/geminidr/interactive/README.md#find-source-apertures (it looks like the name shown there is out of date). I think it has been tidied up since that example plot.

@PatrickOgle
Copy link

(1) We definitely want to compute a trace from an image. It is essential for spectral extraction.
(2) I am not sure why one would want to attach the image to the trace.
(3) In the simple case of independent dispersion and spatial coordinates, the trace function should yield a list of (x,y) coordinates that follow the ridge line of the spectral trace.
(4) For unrectified spectra that mix dispersion and spatial coordinates, the trace object might be a list of (x,y,PA) coordinates, where PA gives the mixing angle between the two coordinates.

@PatrickOgle
Copy link

Regarding a GUI to aid spectral extraction, we plan on having that capability in jdaviz.

@ojustino
Copy link
Contributor Author

Hi @tepickering, how close do you feel to having your new trace class ready for a PR? Our team's product owners would like at least one that can calculate traces by the end of this week.

I can open a new pull request with the KOSMOS implementation since we have James' permission, so no need to distort your schedule if you think it will take longer.

@tepickering
Copy link
Contributor

sorry for the slow response. i was working on the mountain all day and mostly away from a computer (or at least my own computer). i am booked with MMTO stuff thru mid-week and was planning on taking a crack at porting over the DRAGONS implementations of object finding and trace fitting towards the end of the week. if you need something sooner, then do what you need to do. we can start with the KOSMOS implementation and build out from there.

as an aside, i noticed in DRAGONS that they also are hard-coded in places to treat dispersion as being along the "X" axis (modulo distortion) and have hooks to transpose data when/if needed to match that. we may consider a similar approach. it would be nice to be fully generalized, but it could get complicated.

@ojustino
Copy link
Contributor Author

I would say we resolved this issue by finishing up KosmosTrace (now FitTrace on main). Subsequent points can go into new issues.

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

No branches or pull requests

7 participants