AstarVienna / ScopeSim

A telescope observation simulator for Python.
GNU General Public License v3.0
16 stars 10 forks source link

Fix detectorwindow #507

Open hugobuddel opened 1 week ago

hugobuddel commented 1 week ago

Pff together with #506 this fixes https://github.com/AstarVienna/irdb/issues/161 (this P.R. currently adds only 1 commit on top of #506).

The 3rd MICADO notebook now produces figures exactly the same as in February 2022

(You'd need to set !DET.x and !DET.y to 2500 (# pixels, not arcsec) to get it to work.)

However test_happy_using_x_size_unit_in_pixels() is now failing, but I'm not going to look into that right now. Therefore marked as draft.

Progress though!

We can now also create beautiful figures like fvfullstair

codecov[bot] commented 1 week ago

Codecov Report

Attention: Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.

Project coverage is 30.65%. Comparing base (d6758f6) to head (fa34a70).

Files with missing lines Patch % Lines
scopesim/effects/psfs/discrete.py 0.00% 5 Missing :warning:
scopesim/effects/detector_list.py 0.00% 2 Missing :warning:

:exclamation: There is a different number of reports uploaded between BASE (d6758f6) and HEAD (fa34a70). Click for more details.

HEAD has 20 uploads less than BASE | Flag | BASE (d6758f6) | HEAD (fa34a70) | |------|------|------| ||22|2|
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #507 +/- ## =========================================== - Coverage 77.18% 30.65% -46.53% =========================================== Files 66 64 -2 Lines 8205 8041 -164 =========================================== - Hits 6333 2465 -3868 - Misses 1872 5576 +3704 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.


🚨 Try these New Features:

Camillechatenet commented 6 days ago

you need 2500 pixels because the Fov is 10"(for SCAO mode) and each pixel is equal to 4 mas. So why this figure is more than 4000 pixels ?

Camillechatenet commented 6 days ago

you need 2500 pixels because the Fov is 10"(for SCAO mode) and each pixel is equal to 4 mas. So why this figure is more than 4000 pixels ?

hugobuddel commented 6 days ago

you need 2500 pixels because the Fov is 10"(for SCAO mode) and each pixel is equal to 4 mas. So why this figure is more than 4000 pixels ?

That 2500 is the offset (in pixels I think), the size is still 4096 (IIRC).

Also, I'm not sure how I created this particular figure. I shared it because it looks 'interesting' and seems to indicate a bug. The 'step' pattern is also seen in the original notebook, but it is way more pronounced with the updated code.

But note that this branch is still experimental, in particular because the tests don't pass.

hugobuddel commented 6 days ago

Codecov says the new lines in detector_list.py are not covered. Do you think it would be worthwhile to add a test case that goes down that if statement? Especially given that this piece of code apparently produced a bug?

Ah thank you. Maybe the test fail exactly because the test code does not go through that path, but the notebooks work because they do take that branch. No that doesn't make sense. I don't really have time to look into it further now.

Camillechatenet commented 5 days ago

The notebook works! You need to change 10 to 0 for micado.cmds[ "DET.x"] (same for y) line 19

Camillechatenet commented 5 days ago

I suggest also to change cluster properties : mass=200 core_radius=0.005 and distance= 900 to see the same shape of cluster in each image.

hugobuddel commented 5 days ago

The notebook works! You need to change 10 to 0 for micado.cmds[ "DET.x"] (same for y) line 19

Yes, setting DET.x to 0 should work, but that defies the purpose of that part of the notebook. It should be possible to place the DetectorWindow anywhere; in this particular notebook somewhere 'off axis' to show that the PSF changes over the field of view.

I'm not really inclined to spent too much time to continue working on this now though, because simulating the full detector array now seems to work 'fine'. Using the DetectorWindow is just for convenience so the simulation goes faster, but it is not essential.

So I hope you can continue your work @Camillechatenet by using the full detector array, or indeed as you did now the centered detector window.

Fixing #506 was essential though, because that prevented the FieldVaryingPSF to work at all. I'm not convinced the FieldVaryingPSF is working entirely as it should now, so maybe we should investigate that a bit more.

A good reason to fix this DetectorWindow problem anyway is that such bugs might be an indication of larger problems.