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

NXdetector: Add virtual_pixel_correction_applied #942

Merged

Conversation

soph-dec
Copy link
Contributor

We use virtual pixels to account for the space in between chips. For that, the edge pixels of the chips are extended into the gaps, creating more counts on those edges.The virtual pixel correction distributes these counts over the logical pixels.

@PeterC-DLS
Copy link
Contributor

The description in the doc is too succinct. The concept and context of virtual pixels is missing: you need to explain what/where these virtual pixels are, what this correction is and how this relates to gap pixels indicated in pixel_mask.

@soph-dec
Copy link
Contributor Author

I see, I'll do that! I was keeping it short so it would match the descriptions of other fields, like flatfield_correction_applied or countrate_correction_applied.

@soph-dec soph-dec force-pushed the add-virtual-pixel-correction-applied branch 2 times, most recently from 936a500 to 5820508 Compare August 19, 2021 14:41
@PeterC-DLS
Copy link
Contributor

That's a clear explanation. So the consequence of this flag being true is that all pixels labelled as a virtual pixel in the pixel_mask no longer have to be ignored.

@rayosborn
Copy link
Contributor

This looks interesting. Is this for a particular brand of detector and is the correction performed by software the manufacturer supplies?

@kalcutter
Copy link
Contributor

@rayosborn The majority of DECTRIS detectors have virtual pixels. In this case, the interpolation is performed as a correction by the supplied software. I believe there are also some other manufacturers that employ virtual pixels in their detectors.

@kalcutter
Copy link
Contributor

Do people see an issue using "correction" in the name? I suppose if the hardware did virtual pixel interpolation directly, it wouldn't strictly be a "correction". The field could also be called virtual_pixel_interpolation?

@soph-dec
Copy link
Contributor Author

What is needed for this to get approved?

@phyy-nx
Copy link
Contributor

phyy-nx commented Sep 15, 2021

Agree that virtual_pixel_interpolation_applied is a better name. Consensus in NIAC telco is after that change is made this PR is good to go.

@soph-dec soph-dec force-pushed the add-virtual-pixel-correction-applied branch from 5820508 to 73ea20e Compare September 15, 2021 14:51
@yayahjb
Copy link
Contributor

yayahjb commented Sep 16, 2021

Looks good. -- Herbert

@yayahjb yayahjb assigned yayahjb and prjemian and unassigned yayahjb Sep 16, 2021
@prjemian prjemian added this to the NXDL 2022.03 milestone Dec 17, 2021
@phyy-nx phyy-nx added the NIAC vote needed PR needs an approving vote from NIAC before merge label Feb 25, 2022
@prjemian prjemian modified the milestones: NXDL 2022.06, NXDL 2023.06 Jun 24, 2022
@benajamin
Copy link
Contributor

@phyy-nx Merging this branch was approved by NIAC vote (nexusformat/NIAC#94). Is there a good reason why it hasn't been done yet?

@soph-dec soph-dec force-pushed the add-virtual-pixel-correction-applied branch from 73ea20e to 4107140 Compare September 6, 2022 13:59
@soph-dec
Copy link
Contributor Author

soph-dec commented Sep 6, 2022

The merge conflicts have been resolved now.

@phyy-nx
Copy link
Contributor

phyy-nx commented Sep 8, 2022

@benajamin thanks for the reminder and @soph-dec thanks for the fixes. Merging!

@phyy-nx phyy-nx merged commit 01093ec into nexusformat:main Sep 8, 2022
@benajamin benajamin added NIAC approved and removed NIAC vote needed PR needs an approving vote from NIAC before merge labels Sep 17, 2022
@PeterC-DLS PeterC-DLS modified the milestones: NXDL 2023.06, NXDL 2023.10 Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

8 participants