Skip to content

Conversation

@oczoske
Copy link
Collaborator

@oczoske oczoske commented Aug 22, 2024

The ADConversion effect simulates the analogue-digital converter of a detector:

  • it applies the gain factor to convert from electrons to ADU
  • it converts to the desired output data type (e.g. uint16, as often raw data are written with BITPIX=16.

The implementation expands the existing Quantization (which imho is a misnomer).

This PR is accompanied by AstarVienna/irdb#189.

@teutoburg teutoburg added enhancement PR adding or improving a feature (use "Feature" type for issues (requests), not this label) effects Related to a ScopeSim effect labels Aug 22, 2024
@hugobuddel
Copy link
Collaborator

Renaming the Quantization to ADConversion sounds good, because that name is closer to the physical process.

However, if it also applies the gain, then the effect should always be applied, also when NDIT > 1 or when AutoExposure is used, but the quantization part should not. (Or a similar solution.)

This is a draft PR, so you probably already thought of that. I think that is what your todo: need to deal with this case more realistically statement is for, so I expect this comment is superfluous.

@codecov
Copy link

codecov bot commented Aug 22, 2024

Codecov Report

Attention: Patch coverage is 82.85714% with 6 lines in your changes missing coverage. Please review.

Project coverage is 76.81%. Comparing base (8ad235b) to head (ee9541b).
Report is 350 commits behind head on main.

Files with missing lines Patch % Lines
scopesim/effects/fits_headers.py 57.14% 3 Missing ⚠️
scopesim/effects/electronic/electrons.py 92.00% 2 Missing ⚠️
scopesim/effects/electronic/dmps.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #455      +/-   ##
==========================================
+ Coverage   76.74%   76.81%   +0.06%     
==========================================
  Files          70       70              
  Lines        8574     8599      +25     
==========================================
+ Hits         6580     6605      +25     
  Misses       1994     1994              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@teutoburg teutoburg mentioned this pull request Nov 7, 2024
@teutoburg teutoburg added the Pipeline dev. Technical target audience label Jan 31, 2025
@teutoburg teutoburg linked an issue Mar 25, 2025 that may be closed by this pull request
@oczoske oczoske marked this pull request as ready for review April 1, 2025 14:10
@oczoske
Copy link
Collaborator Author

oczoske commented Apr 1, 2025

Still not entirely comfortable with this, but that may have more to do with the uncertainty of how our detectors (in METIS and MICADO) are actually read out and written to files rather than with the code itself. Also our sparse knowledge of actual gain values is more of a matter for irdb than for Scopesim.

@teutoburg
Copy link
Contributor

Mentioned this offline, but just so we don't forget: This should be in the next minor version, because it will break IRDB packages still referring to a Quantization effect. If we want to make another patch release before, we should wait before merging this.

@oczoske oczoske requested a review from teutoburg April 22, 2025 15:28
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.

We talked about this for long enough I think, let's go 👍

Comment on lines 167 to 169
#if not np.issubdtype(new_dtype, np.integer):
# logger.warning("Setting digitized data to dtype %s, which is not "
# "an integer subtype.", new_dtype)
Copy link
Contributor

Choose a reason for hiding this comment

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

To comment on the "(why?)" in the commit message: IIRC the original idea with the Quantization (sic!) was that it should normally produce an integer dtype, because it's "counting whole electrons" or something. So I think the warning was along the lines of "You're applying quantization to produce whole integer numbers of something but you're not actually creating integers."

But I'm fine with removing this, now that we don't call it Quantization any more. I guess the ADConversion can conceptually produce fractional numbers in the case of an average exposure, so probably the original thought behind this warning is no longer valid anyway.

@oczoske oczoske merged commit 87961e7 into main Apr 25, 2025
23 checks passed
@oczoske oczoske deleted the oc/adconversion branch April 25, 2025 13:35
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in ScopeSim-development Apr 25, 2025
@hugobuddel
Copy link
Collaborator

Thank you @oczoske ! This is so much better and more natural than what we had.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effects Related to a ScopeSim effect enhancement PR adding or improving a feature (use "Feature" type for issues (requests), not this label) Pipeline dev. Technical target audience

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Rename Quantization to ADConversion

4 participants