Skip to content

Conversation

@JenniferKarr
Copy link
Contributor

remove a .lower() on the model name for stellar models. Some of the SpeXtra models (e.g. irtf) have upper case names for the spectral stypes, which can't be accessed via the current code.

…peXtra models (e.g. irtf) have upper case names for the spectral stypes, which can't be accessed via the current code.
@codecov
Copy link

codecov bot commented Apr 10, 2025

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 74.25%. Comparing base (2995a6e) to head (dcbdc54).

Files with missing lines Patch % Lines
scopesim_templates/stellar/stars.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #135   +/-   ##
=======================================
  Coverage   74.25%   74.25%           
=======================================
  Files          30       30           
  Lines        1484     1484           
=======================================
  Hits         1102     1102           
  Misses        382      382           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@hugobuddel
Copy link
Collaborator

Could you perhaps add a test for this to test_stars.py. Something that fails without this patch, but works with the patch. I expect something simple like this :

    def test_irtf(self):
        src = stars("Generic/Johnson.V", [0]*u.mag, ["A0V"], [0], [0], library="irtf")

and then maybe assert that src is of the expected class.

I'm asking because that lower() might have been there on purpose, but all tests pass, so if it was intentional was no test for that purpose. We don't want future us to be annoyed when we break this again, so we need to add a test.

I wouldn't know what to put on that line to trigger the problem. So I would myself not be able to add a test later.

@teutoburg teutoburg changed the title fixed a case conversion issue in accessing SpeXtra Fix a case conversion issue in accessing SpeXtra Apr 10, 2025
@teutoburg
Copy link
Contributor

    def test_irtf(self):
        src = stars("Generic/Johnson.V", [0]*u.mag, ["A0V"], [0], [0], library="irtf")

This does indeed fail on main with NotInLibraryError: Spectral template 'a0v' not in library, but that might be because the IRTF library only goes up to F. From this branch, using "F0V" instead, we get another error: DisjointError: Source spectrum and bandpass are disjoint. (unlike on main, where the error is the same as for "A0V"). So yeah, this PR seems to solve the lowercase issue, but there are other problems 🙃

@hugobuddel
Copy link
Collaborator

    def test_irtf(self):
        src = stars("Generic/Johnson.V", [0]*u.mag, ["A0V"], [0], [0], library="irtf")

This does indeed fail on main with NotInLibraryError: Spectral template 'a0v' not in library, but that might be because the IRTF library only goes up to F. From this branch, using "F0V" instead, we get another error: DisjointError: Source spectrum and bandpass are disjoint. (unlike on main, where the error is the same as for "A0V"). So yeah, this PR seems to solve the lowercase issue, but there are other problems 🙃

Yes, that was my experience too. Those other things are 'normal' behavior for speXtra (or problems, but unrelated to this PR). So I couldn't figure out how to test this. @JenniferKarr could you comment on how to test this?

@hugobuddel
Copy link
Collaborator

What is the status of this PR @JenniferKarr ? You didn't assign a reviewer, nevertheless both me and @teutoburg added some comments, so it is not clear to me who is waiting for who.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 🚧 On hold

Development

Successfully merging this pull request may close these issues.

4 participants