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

Implement apply decision for quantization #396

Merged
merged 15 commits into from
Apr 11, 2024
Merged

Implement apply decision for quantization #396

merged 15 commits into from
Apr 11, 2024

Conversation

teutoburg
Copy link
Contributor

Closes #306

@teutoburg teutoburg added enhancement New feature or request effects Related to a ScopeSim effect labels Apr 10, 2024
@teutoburg teutoburg self-assigned this Apr 10, 2024
Copy link

codecov bot commented Apr 10, 2024

Codecov Report

Attention: Patch coverage is 76.56250% with 15 lines in your changes are missing coverage. Please review.

Project coverage is 74.85%. Comparing base (6057f73) to head (b943451).
Report is 3 commits behind head on dev_master.

Files Patch % Lines
scopesim/effects/electronic.py 76.56% 15 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff               @@
##           dev_master     #396      +/-   ##
==============================================
- Coverage       74.92%   74.85%   -0.08%     
==============================================
  Files              56       56              
  Lines            7777     7818      +41     
==============================================
+ Hits             5827     5852      +25     
- Misses           1950     1966      +16     

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

@teutoburg
Copy link
Contributor Author

The Quantization effect needs to be actually included in the IRDB (mostly METIS for now). I did try that locally and it seems to work with the following examples passed to the properties of the corresponding UserCommands of a simple METIS observation:

  • "!OBS.exptime": None or "!OBS.exptime": 5 results in floats because DIT & NDIT are auto-determined by the AutoExposure
  • "!OBS.ndit": 5, "!OBS.dit": 5, "!OBS.exptime": None results in floats because NDIT > 1
  • "!OBS.ndit": 1, "!OBS.dit": 5, "!OBS.exptime": None results in integers because NDIT == 1

In this case, not supplying any DIT and NDIT will also result in floats, because the default DIT set in the IRDB is increased based on mindit by the AutoExposure, thus resulting in "automatically determined DIT". So only when DIT and NDIT are both supplied with NDIT == 1, and pass mindit, the quantization takes place. I guess that's what we want?

This wouldn't have been necessary with better chainmap stuff...
@teutoburg teutoburg marked this pull request as ready for review April 10, 2024 18:13
@teutoburg
Copy link
Contributor Author

teutoburg commented Apr 11, 2024

One slight complication about all this: exptime, dit and ndit all have default values (see IRDB/METIS/default.yaml). There's not really a (non-hacky) way for the code to know if any (non-None) values found for these are coming from the defaults or from user input. Removing the values from the defaults might break things and will then also require the user to always specify either exptime or dit & ndit. Which is probably good, but extends the necessary overhead and reduces the ability to "just quickly run it".

Possibly solution, at least for METIS (or anything else using AutoExposure:
Put the defaults for exptime, dit and ndit to either 0 or None (which is better conceptually??) and include the following check in AutoExposure: If both exptime and at least one of dit & ndit are nonset, issue a warning but use a best-guess to still run the simulation regardless. This could be MINDIT if it's present, or otherwise just 1 s.

Thoughts @hugobuddel ?

Plus some extras to make sure edge cases are covered.
Use meaningful default if no exptime given.
Also include lots of logging.
@teutoburg
Copy link
Contributor Author

Possibly solution, at least for METIS (or anything else using AutoExposure:
Put the defaults for exptime, dit and ndit to either 0 or None (which is better conceptually??) and include the following check in AutoExposure: If both exptime and at least one of dit & ndit are nonset, issue a warning but use a best-guess to still run the simulation regardless. This could be MINDIT if it's present, or otherwise just 1 s.

After discussion with @astronomyk we decided to go ahead with this "solution" for now.

@teutoburg teutoburg merged commit 89dd583 into dev_master Apr 11, 2024
25 checks passed
@teutoburg teutoburg deleted the fh/kwant branch April 11, 2024 15:30
@hugobuddel
Copy link
Collaborator

Sounds good! I didn't take those defaults into account in my thought process.

Maybe we could set either exptime or dit/ndit to 0/None by default but not both? Then at least users get something interesting by default. All three are 1 now in METIS/default.yaml. I suggest keeping exptime default at 1, and set dit/ndit at 0/None, because then people who do not change any defaults would get the most useful result.

And all of this should be kinda part of a hypothetical ETC-simulator (or actual ETC?), which can then be coupled to ScopeSim somehow.

@hugobuddel
Copy link
Collaborator

It seems this MR is the cause of #438; investigating.

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 New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Let ScopeSim simulate raw exposures as unsigned integers, or floats with specified precision.
2 participants