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
Fix sunpy.io to update header info after scaling applied when data is accessed #7141
base: main
Are you sure you want to change the base?
Conversation
8c8df4c
to
71194c4
Compare
sunpy/io/_fits.py
Outdated
try: | ||
hdu.data # access data to force any scaling BSCALE/BZERO to be applied and keyword will be updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I worry about the performance impact and general side-effects of doing this here. 🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea I was too but I don't think it's any different to this line which was and still is being called HDPair(hdu.data, header)
. Same side effect as far as I can see, hdu.data
is being accessed and causes astropy.io to load and scale the data, I'll try to confirm either way. Long term if you want to support lazy loading and dask etc this is an issue for sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to request changes since I'm not quite sure what this is fixing. It should have a test anyway, and I think this would help review the PR.
71194c4
to
02f5d86
Compare
EDIT: I MISINTERPRETED THE BUG REPORT - SKILL ISSUE ON MY PART |
02f5d86
to
5fda1f3
Compare
6d1c429
to
0d9e327
Compare
0d9e327
to
165a078
Compare
Initial quick fix
_fits.get_header
and_fis.read_fits
could be refactored to make this a but nicer will do later.