Skip to content

Simplify checks for 1D/2D spectra and traces#4176

Open
rosteen wants to merge 3 commits into
spacetelescope:mainfrom
rosteen:data-type-in-meta
Open

Simplify checks for 1D/2D spectra and traces#4176
rosteen wants to merge 3 commits into
spacetelescope:mainfrom
rosteen:data-type-in-meta

Conversation

@rosteen
Copy link
Copy Markdown
Collaborator

@rosteen rosteen commented May 12, 2026

Now checks for the importer in these cases like a lot of the other is_valid checks, and additionally adds a _data_type field in meta to distinguish cases where a collapsed 1D spectrum is being loaded by the 2D or 3D spectrum importer. This is purely internal so I don't think it needs a changelog, and it should be covered by existing tests.

@rosteen rosteen added this to the 5.1 milestone May 12, 2026
@rosteen rosteen added the 💤 enhancement New feature or request label May 12, 2026
@rosteen rosteen added the no-changelog-entry-needed changelog bot directive label May 12, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 12, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.74%. Comparing base (02ddc71) to head (e2a1e8e).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4176      +/-   ##
==========================================
- Coverage   84.76%   84.74%   -0.03%     
==========================================
  Files         205      205              
  Lines       30552    30554       +2     
==========================================
- Hits        25897    25892       -5     
- Misses       4655     4662       +7     

☔ 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.

Comment on lines +200 to +201
data_type = None

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is, on first glance, confusing... any objection to moving the data_type instantiations into __init__ via self.data_type = ... for all of these importers?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

You know, it made more sense when I was going to check data_type for all of the validity checks instead of sticking with looking at _importer in most cases. I think I can simplify this for now since the only times I'm looking at _data_type are in the override cases, and not set it on the importer class at all. Unless it's something we do want to start adding to meta for every importer.

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

Labels

no-changelog-entry-needed changelog bot directive 💤 enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants