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

[WIP] Rough implementation of some non-linear loaders #192

Merged
merged 5 commits into from
Aug 7, 2019

Conversation

simontorres
Copy link
Contributor

I have entered the overall structure for the loaders of non-linear wavelength solutions using the iraf convention, though I'm affraid I still don't have the definite (official) reference for the standard documentation but I think it has to be this paper by Francisco Valdes

The functions were copied and adapted from the goodman pipeline thefore the docstrings might not apply.

I would also like some feedback for the following topics.

  • The current format wcs1d-fits is capable of loading linear-solutions only and not log-linear. If I'm going to implement the loader iraf it requires to have log-linear and therefore
  • wouldn't this be better implemented in specutils/wcs/wcs_wrapper.py? then less loaders are required and WCS (wrapper) takes care of the rest.
  • any feedback will be appreciated.

@nmearl
Copy link
Contributor

nmearl commented May 6, 2018

The current wcs1d-fits can indeed load non-linear solutions. While the WCS implementation in astropy does not seem to support TNX WCS formalism, it does support SIP and distortion table look-up transformations (which in section 3.1 describes Legendre, Chebychev, cubic, etc polynomial representations). It may be enough to simply re-compose the WCS object in the loader into a format that astropy understands.

wcs_wrapper.py is nothing but an abstract class that defines the methods for interfacing with FITSWCS and GWCS objects. If non-linear support truly needs to be built-in to specutils, it would be done in the fits_adapter.py class. Perhaps something like a function to convert a TNX WCS into a WCS astropy natively understands using the above papers might be worth putting in there.

@bsipocz
Copy link
Member

bsipocz commented May 9, 2018

@simontorres - Since the code was copied over from a GPL licenced project, could you please add a note in the source code here that the copyright owner is OK with this (and the fact that it'll be relicenced under BSD-3)?

@simontorres
Copy link
Contributor Author

@bsipocz thanks for the notice, in fact we had decided to release the Goodman Pipeline under BSD-3 but for some reason GPL stayed in place (my responsibility though). We are preparing a new release under BSD-3.

@nmearl I'm thinking if this is worth the effort. Certainly would be nice that specutils can handle as much formats as they are out there, the question is how many people is going to use it. If specutils can write a non-linear solution using the header, not tabular data, I would switch right away and publish my library in a separate repo just in case someone needs it someday.

@crawfordsm
Copy link
Member

I would be in favor of merging this. If we decide later to handle this in a different manner (especially with any future updates to astropy wcs), then it can be refactored but in the meantime it provides useful functionality to read in a common format while returning a Spectrum1D object.

@simontorres
Copy link
Contributor Author

In that case I can resume working on this.

@nmearl nmearl self-requested a review September 5, 2018 15:37
@nmearl nmearl added this to the v0.6.0 milestone Sep 5, 2018
@nmearl
Copy link
Contributor

nmearl commented Sep 7, 2018

@simontorres Can you add a test for this loader?

@eteq eteq modified the milestones: v0.4.0, v0.5.0 Sep 13, 2018
@hamogu
Copy link
Member

hamogu commented Nov 27, 2018

I recommend to look back at #14, #17, #18 which did implement a bunch of readers, including non-linear WCS systems. Astropy has evolved since and some of the code in those PRs may not be necessary any longer because the WCS now supports it out of the box. However, the discussion in the PR should also include references for the documented format, and there were test cases.

I wrote those readers (and I remember it as a very painful process), but not in removing them again from this package. Please forgive me for not volunteering to help implement the same thing a second time after the community chose to remove my contributions in some later PR (in commit 9d56a31).

specutils/io/default_loaders/wcs_fits_reader.py Outdated Show resolved Hide resolved
specutils/io/default_loaders/wcs_fits_reader.py Outdated Show resolved Hide resolved
specutils/io/default_loaders/wcs_fits_reader.py Outdated Show resolved Hide resolved
specutils/io/default_loaders/wcs_fits.py Show resolved Hide resolved
specutils/io/default_loaders/wcs_fits.py Outdated Show resolved Hide resolved
specutils/io/default_loaders/wcs_fits.py Outdated Show resolved Hide resolved
specutils/io/default_loaders/wcs_fits.py Outdated Show resolved Hide resolved
specutils/io/default_loaders/wcs_fits.py Outdated Show resolved Hide resolved
specutils/tests/test_loaders.py Outdated Show resolved Hide resolved
specutils/tests/test_loaders.py Outdated Show resolved Hide resolved
specutils/tests/test_loaders.py Outdated Show resolved Hide resolved
specutils/tests/test_loaders.py Outdated Show resolved Hide resolved
specutils/tests/test_loaders.py Outdated Show resolved Hide resolved
@nmearl nmearl merged commit 7eaeb57 into astropy:master Aug 7, 2019
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.

None yet

6 participants