[Jungfrau] New condition exposure timout

Description

This is a draft for now as it is rebased on the under-review MR for injecting dark CCVs

This addresses new condition for Jungfrau Exposure Timout

How Has This Been Tested?

• Reference runs

Relevant Documents (optional)

Types of changes

• New feature (non-breaking change which adds functionality)

Checklist:

Reviewers

@schmidtp @mramilli

assigned to @ahmedk

changed the description

added Test CAL Data Affected Testing labels

added 1 commit

• 17900631 - fix: update calcat_interface and test and make sure new condition is still backward compatabile

Compare with previous version

marked this merge request as draft

added 1 commit

• 5d8c2a82 - fix: update test_calcat_interface

Compare with previous version

Is 0 our default, or possibly some other non-zero value? If so, we should do the same as for AGIPD integration time.

Exposure Timer is a read only parameter whose default value is 0.

Then I'm confused about this earlier statement:

this parameter allows to change the dead time in between acquisition gates when JF is used in burst mode;

the parameter which is changing the dead time is the Exposure Timeout Exposure Timeout is in nanoseconds for user-fiendliness, Exposure Timer is in number of clocks.

Brilliant. I actually thought it to be timeout initially, but double checked Marcin's email and it clearly stated "Timer". I only checked the property exists, not it's reconfigurable...

@ahmedk Sorry for that, but better we figure this out now than later.

changed this line in version 8 of the diff

To confirm. I have set the exposure timeout default value to 25. It is the same as we did with AGIPD integration time 12. To be backward compatible with previous data.

Beautiful small diff with the new API(s), thanks!

Looking at this, it's quite clear the next step should be to use the condition data classes from the start to track control parameters, with a nice .from_data() method. But that's for the future.

LGTM pending the underlying functionality MR.

added 55 commits

• 5d8c2a82...1b2a1192 - 47 commits from branch feat/remove_calibrationDBRemote_usage
• 11de28b3 - fix: small fixes after testing part of injecting ccvs
• 0001ae79 - fix: handle CCVAlreadyInject case using a warning
• cd3a8ae2 - fix: Add a partial convenient CCVAlreadyInjectedError for now in this implementation
• fe25c2b6 - feat: second round of resolving discussions
• 2d924bd8 - draft: adding new exposure_timer for jungfrau correct/dark notebooks
• 097022a0 - feat: update name and datasource to check before testing
• e1a1024d - fix: update calcat_interface and test and make sure new condition is still backward compatabile
• e2fcce03 - fix: update test_calcat_interface

Compare with previous version

added 1 commit

• cd96c8ab - fix: remove redundant extra_calibration_client

Compare with previous version

deleted the feat/remove_calibrationDBRemote_usage branch. This merge request now targets the master branch

marked this merge request as ready

changed milestone to %3.15.2

changed title from Feat/new jungfrau exposure timer to [Jungfrau] New condition exposure timout

changed the description

added 40 commits

• cd96c8ab...9012640a - 30 commits from branch master
• c2e75a3f - fix: small fixes after testing part of injecting ccvs
• e033ccc3 - fix: handle CCVAlreadyInject case using a warning
• ebdfae5a - fix: Add a partial convenient CCVAlreadyInjectedError for now in this implementation
• 8c9c5186 - feat: second round of resolving discussions
• 93958cf9 - draft: adding new exposure_timer for jungfrau correct/dark notebooks
• 3afea677 - feat: update name and datasource to check before testing
• 3f2ce112 - fix: update calcat_interface and test and make sure new condition is still backward compatabile
• ca62ac47 - fix: update test_calcat_interface
• 3c939a57 - fix: remove redundant extra_calibration_client
• 962a490f - Exxposure timeout instead of Exposure timer

Compare with previous version

marked this merge request as draft from 93958cf9

marked this merge request as ready

I have updated the condition name to exposure timeout

added 1 commit

• 873ff5ae - have 25 as default exposure timeout for bacward compatibility

Compare with previous version

changed the description

Just to confirm again from @mramilli , the property to introduce as a new parameter is:

• exposureTimeout
• default value: 25 ns (which is not injected)

I was about to comment on this for not triggering when it's missing... until I realized naturally you'd not want to remove it in this case. As it happens, this is actually a bug in !1089 (merged)!

So, I copy pasted this without thinking much about it. But now when you have mentioned it. I guess we should do

if int(cond.get("Exposure timeout", 25)) == 25

This will avoid using -1 if exposure timeout is missing correct?

Quite the opposite: if Exposure timeout is not a key in the dict, it would return 25, the branch evaluates to True and it'd try to delete the key throwing an exception.

This is rarely a problem if said parameter is part of all conditions, but it could be. So your way is the correct way.

Ah, an alternative would be to do cond.pop() with a default value

LGTM (again!).

I've asked Marco one last question if this property has been there all the time, but let's go with it for now.

Checking very old data (test proposal 900113) from 2019. It doesn't have exposureTimeout

Yes, Marco suspects it appeared in late-2019 or early-2020. That is good enough for me, data from befoere that period is tough luck anyway.

Thank you for the review!

merged

mentioned in commit 70533b02

mentioned in merge request !1096 (merged)