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

Fix detectorwindow #507

Merged
merged 4 commits into from
Dec 5, 2024
Merged

Fix detectorwindow #507

merged 4 commits into from
Dec 5, 2024

Conversation

hugobuddel
Copy link
Collaborator

Pff together with #506 this fixes AstarVienna/irdb#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

@hugobuddel hugobuddel marked this pull request as draft November 18, 2024 22:18
Copy link

codecov bot commented Nov 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.00%. Comparing base (2d42d74) to head (ebd354c).
Report is 5 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #507   +/-   ##
=======================================
  Coverage   76.99%   77.00%           
=======================================
  Files          66       66           
  Lines        8213     8216    +3     
=======================================
+ Hits         6324     6327    +3     
  Misses       1889     1889           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Camillechatenet
Copy link

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 ?

1 similar comment
@Camillechatenet
Copy link

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 ?

@teutoburg teutoburg added the bugfix PR resolving one or more bugs label Nov 19, 2024
Copy link
Contributor

@teutoburg teutoburg left a comment

Choose a reason for hiding this comment

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

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?

@hugobuddel
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link

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

@Camillechatenet
Copy link

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
Copy link
Collaborator Author

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.

hugobuddel and others added 4 commits November 29, 2024 15:57
Most likely this was a copy-paste mistake, which resulted in overwriting the previous definition of this method.
Also make this more robust, because x/y_cen_unit is actually used in multiple places in the IRDB. It's currently always mm, but doesn't have to be, so silently assuming mm is just asking for trouble...
@teutoburg teutoburg self-assigned this Dec 3, 2024
@teutoburg
Copy link
Contributor

I fixed the failing test case. I also rebased this onto main, because this was a superset of another PR anyway. @hugobuddel, are you cool with me force-pushing that and then merging once it passed everything (which it does locally)? 🙂

@hugobuddel
Copy link
Collaborator Author

I fixed the failing test case. I also rebased this onto main, because this was a superset of another PR anyway. @hugobuddel, are you cool with me force-pushing that and then merging once it passed everything (which it does locally)? 🙂

Apparently github is not smart enough to realize that the other PR has been merged. Maybe making a (dummy) commit would update the diff. Nevertheless feel free to force-push fix this, as I don't have enough time to work on this issue.

@teutoburg teutoburg force-pushed the hb/fixdetectorwindow branch from fa34a70 to ebd354c Compare December 5, 2024 16:04
@teutoburg teutoburg marked this pull request as ready for review December 5, 2024 16:05
@teutoburg teutoburg merged commit a43ac16 into main Dec 5, 2024
22 checks passed
@teutoburg teutoburg deleted the hb/fixdetectorwindow branch December 5, 2024 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix PR resolving one or more bugs
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

MICADO notebook 3_scopesim_SCAO_4mas_fv-psf.ipynb is broken
3 participants