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

make spectrum summary plot #138

Merged
merged 7 commits into from
Sep 22, 2023
Merged

make spectrum summary plot #138

merged 7 commits into from
Sep 22, 2023

Conversation

jeremyneveu
Copy link
Collaborator

No description provided.

@coveralls
Copy link

coveralls commented Sep 13, 2023

Pull Request Test Coverage Report for Build 6273713403

  • 87 of 90 (96.67%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 90.656%

Changes Missing Coverage Covered Lines Changed/Added Lines %
spectractor/extractor/spectroscopy.py 9 10 90.0%
spectractor/extractor/spectrum.py 78 80 97.5%
Totals Coverage Status
Change from base Build 6170832100: 0.2%
Covered Lines: 6801
Relevant Lines: 7502

💛 - Coveralls

spectractor/extractor/spectroscopy.py Show resolved Hide resolved
spectractor/extractor/spectroscopy.py Show resolved Hide resolved
spectractor/extractor/spectrum.py Outdated Show resolved Hide resolved
Comment on lines +538 to +544
if not np.any([line.fitted for line in self.lines.lines]):
fwhm_func = interp1d(self.chromatic_psf.table['lambdas'],
self.chromatic_psf.table['fwhm'],
fill_value=(parameters.CALIB_PEAK_WIDTH, parameters.CALIB_PEAK_WIDTH),
bounds_error=False)
detect_lines(self.lines, self.lambdas, self.data, self.err, fwhm_func=fwhm_func,
calibration_lines_only=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This whole block feels a bit surprising/scary to me. If there's nothing fitted yet, I think failing/returning False or something would be fine, or just plotting what you can too, without the calibrations applied, but actually detecting lines inside a plotting function is a bit weird, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, but the fitted results are not stored in the .fits files. So when I load a Spectrum from a FITS I need to refit the lines. Should not be a problem for AuxTel.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, fine by me 🙂

spectractor/extractor/spectrum.py Outdated Show resolved Hide resolved
spectractor/tools.py Outdated Show resolved Hide resolved
spectractor/tools.py Show resolved Hide resolved
@jeremyneveu jeremyneveu merged commit 63708ab into master Sep 22, 2023
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.

3 participants