search

OttoStruve / muler

A Python package for working with pipeline-produced spectra from IGRINS, HPF, and Keck NIRSPEC
https://muler.readthedocs.io
MIT License
15 stars 9 forks source link

Roll back resample_list? #56

Closed gully closed 2 years ago

gully commented 3 years ago

Hi @kfkaplan, I have some questions about the use cases for the resample_list method just to make sure I am understanding it correctly.

I just double checked the placement of resample_list method and see that it is in EchelleSpectrum. I think it might need to be relocated from EchelleSpectrum to one of two new locations. It's really an operation on a SpectrumList, so it could/should be moved to EchelleSpectrumList. Alternatively, it is an operation on a model, in which case it should be moved to gollum's precomputed spectrum object. Which of these two (or separate?) use case do you envision?

We currently expect most EchelleSpectrum objects to be a single order spectrum, in which case resampling that to a list does not make as much sense, if the list is composed of spectra of distinct spectral regions. Can you share a tutorial or example of how you would use the resample_list?

kfkaplan commented 3 years ago

It was originally intended to resample a single spectrum it is called onto the wavelength grid of a provided EchelleList object and called something like this: ResampledList = SingleSpectrum.resample_list(ListToResampleTo)

The same functionality is provided by what is now in utilities.py which would be called like this ResampledList = resample_list(SingleSpectrum, ListToResampleTo)

It could be moved from the EchelleList to Echelle class and called like so: ResampledList = ListToResampleTo.resample_list(SingleSpectrum)

It probably is easiest to just remove the method from Echelle and leave it as a stand alone definition in utilities.py to avoid confusion.

kfkaplan commented 3 years ago

I will just remove it in my next pull request.

kfkaplan commented 2 years ago

Wow I totally forgot about this. I just removed resample_list from echelle.py in my latest pull request Updates to normalize() https://github.com/OttoStruve/muler/pull/81. The definition in utilities.py should be the one everyone uses.

kfkaplan commented 2 years ago

This has been resolved in pull request #81 which is now merged with Main. resample_list was moved to utilities.py as its own definition.