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

extract from imagehdu so that full fov is covered #238

Merged
merged 2 commits into from
Jun 29, 2023

Conversation

oczoske
Copy link
Collaborator

@oczoske oczoske commented Jun 28, 2023

This fixes #211, compare
micado_vs_psf_ncpa_ycut_correct.pdf to the figure linked there. Instead of looking at the pixel edges I replaced round() by the more appropriate floor() and ceil() functions and made sure that the proper pixel range is included.

@oczoske
Copy link
Collaborator Author

oczoske commented Jun 28, 2023

Incidentally, extract_area_from_imagehdu relies on functionality that requires overhaul in connection with the off-by-one error. imp_utils.header_from_list_of_xy seems dodgy as well, though apparently not critical.

@oczoske oczoske marked this pull request as draft June 28, 2023 15:08
@oczoske
Copy link
Collaborator Author

oczoske commented Jun 28, 2023

Oh dear, failing tests. These need to be sorted.

@codecov
Copy link

codecov bot commented Jun 28, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.26 ⚠️

Comparison is base (79d0aea) 75.41% compared to head (4566eea) 75.15%.

Additional details and impacted files
@@              Coverage Diff               @@
##           dev_master     #238      +/-   ##
==============================================
- Coverage       75.41%   75.15%   -0.26%     
==============================================
  Files             149      152       +3     
  Lines           15283    15668     +385     
==============================================
+ Hits            11525    11775     +250     
- Misses           3758     3893     +135     
Impacted Files Coverage Δ
scopesim/optics/fov_utils.py 63.04% <100.00%> (+0.82%) ⬆️

... and 12 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@oczoske
Copy link
Collaborator Author

oczoske commented Jun 28, 2023

Sorted. The function needed protection against accessing pixels outside the ImageHDU.

@oczoske oczoske marked this pull request as ready for review June 28, 2023 20:49
@hugobuddel
Copy link
Collaborator

Thank you @oczoske. Tagging #209. I'll merge.

Maybe this missing protection against accessing pixels outside the ImageHDU was also the root cause of the MICADO notebooks failing for 4k*4k simulations back when you first tried to fix the off-by-one-error. Unfortunately I cannot now find the issue / PR where we discussed that.

That is, the original rounding code would usually round in the 'right' direction, but occasionally in the 'wrong' direction, which would only happen for simulations of large detectors. Your protection would probably fix that, so one step closer to doing this properly.

@hugobuddel hugobuddel merged commit 6ad75c5 into dev_master Jun 29, 2023
7 of 8 checks passed
@oczoske oczoske deleted the oc/211/psf_problem branch June 30, 2023 11:03
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.

NCPA and PSF affect MICADO spectroscopy with homogeneously filled slit in a weird way
3 participants