Skip to content

Allow specifying flux column in JWST x1d reader #1244

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

Merged
merged 2 commits into from
Jul 14, 2025

Conversation

rosteen
Copy link
Contributor

@rosteen rosteen commented Jun 27, 2025

Closes #1144 . Unfortunately it's not possible to do this across all readers simultaneously, so I implemented it in the JWST reader since that's the one that is causing problems. If this comes up for other missions we can add the keyword to those as well.

@rosteen rosteen added this to the v2.0 milestone Jun 27, 2025
@rosteen rosteen requested review from eteq and keflavich as code owners June 27, 2025 16:05
@rosteen rosteen added the io label Jun 27, 2025
@keflavich
Copy link
Contributor

lgtm so far, but why isn't this possible more generally? you can just do:

flux = Quantity(data[flux_column])

(and then optionally error = Quantity(data[error_column])) and leave it to the user to specify the column names correctly.

I like the approach here for JWST data and approve of doing this sort of extra hand-holding whenever possible, but on a broader level, the user should be able to override any choices imposed by the registered reader.

@rosteen
Copy link
Contributor Author

rosteen commented Jul 2, 2025

but why isn't this possible more generally?

It's not that it isn't possible more generally, it's that we rely on NDIOMixin for the read method, so there's not (I don't think anyway) a central place to add this just once to apply to all the loaders, we would need to add it to each of the loaders individually. I'm happy to take the broader effort to do that, but I figured I'd start with the case that was actually causing problems.

@rosteen rosteen force-pushed the add-flux-col-kwarg branch from 49b0df0 to deb59e8 Compare July 8, 2025 17:38
@rosteen
Copy link
Contributor Author

rosteen commented Jul 14, 2025

I'm going to go ahead and merge this, we can follow up to extend this to other loaders if desired.

@rosteen rosteen merged commit f030db6 into astropy:main Jul 14, 2025
11 of 12 checks passed
@kelle
Copy link
Member

kelle commented Jul 17, 2025

I'm not sure if this is asking too much but can we get a release with this fix please?

@rosteen
Copy link
Contributor Author

rosteen commented Jul 22, 2025

I'm not sure if this is asking too much but can we get a release with this fix please?

I'll try to get a release out later this week, since there have been a couple PRs merged. I'm trying to write tests for #1252 and get it in before I do a release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add flux column keyword to Spectrum.read
3 participants