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

DM-46003: Increase minimum exposure time for electrometer in ATCalsys #156

Merged
merged 1 commit into from
Sep 6, 2024

Conversation

parfa30
Copy link
Contributor

@parfa30 parfa30 commented Aug 27, 2024

electrometer exposure time must be a minimum of 1 second with Keithley electrometer.

@parfa30 parfa30 requested a review from tribeiro August 27, 2024 20:42
Copy link
Member

@tribeiro tribeiro left a comment

Choose a reason for hiding this comment

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

Looks ok, I am going to approve the PR but please, before merging, replace the hard coded 1s with an attribute. This can be a method attribute so long as it is not hard coded inline (and repeated 3 times).

@@ -317,13 +317,19 @@ async def _calculate_electrometer_exposure_times(
* electrometer_time_separation_vs_integration
) + electrometer_integration_overhead
max_exp_time = electrometer_buffer_size * time_sep
if exptime > max_exp_time:
electrometer_exptimes.append(max_exp_time)
if exptime < 1.0:
Copy link
Member

Choose a reason for hiding this comment

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

can you make this "minimum exposure time" an attribute like the others at the top of the method, e.g. eletrometer_mini_exp_time = 1.0?

if exptime > max_exp_time:
electrometer_exptimes.append(max_exp_time)
if exptime < 1.0:
elec_exptime = 1.0
Copy link
Member

Choose a reason for hiding this comment

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

replace 1.0 with electrometer_min_exp_time.

if exptime < 1.0:
elec_exptime = 1.0
self.log.info(
f"Electrometer exposure time increased from {exptime} to 1 sec."
Copy link
Member

Choose a reason for hiding this comment

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

Replace "1 sec" with `{electrometer_min_exp_time} sec".

@parfa30 parfa30 force-pushed the tickets/DM-46003 branch 2 times, most recently from 8055702 to 02073c2 Compare August 27, 2024 23:20
@parfa30 parfa30 merged commit 1c64a8a into develop Sep 6, 2024
5 checks passed
@parfa30 parfa30 deleted the tickets/DM-46003 branch September 6, 2024 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants