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

Add functionalities to dpo measurement #1656

Merged

Conversation

sohailc
Copy link
Member

@sohailc sohailc commented Aug 5, 2019

We add the following functionalities to the measurement submodule of the driver:

  1. Ability to gather measurement statistics: mean, max, min and standard deviation
  2. Automatically turn a measurement on when calling a measurement method (Set state to True)
  3. Ability to globally set the statistics mode, time constant and buffer resetting

@astafan8 Can you have a look, please?

@codecov
Copy link

codecov bot commented Aug 5, 2019

Codecov Report

Merging #1656 into master will increase coverage by 0.03%.
The diff coverage is 87.5%.

@@            Coverage Diff             @@
##           master    #1656      +/-   ##
==========================================
+ Coverage   67.01%   67.04%   +0.03%     
==========================================
  Files         143      143              
  Lines       17645    17677      +32     
==========================================
+ Hits        11825    11852      +27     
- Misses       5820     5825       +5

Copy link
Collaborator

@jenshnielsen jenshnielsen left a comment

Choose a reason for hiding this comment

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

I have no experience with this scope but the changes seems sensible enough to me

"time_constant",
get_cmd="MEASUrement:STATIstics:WEIghting?",
set_cmd="MEASUrement:STATIstics:WEIghting {}",
get_parser=int
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not obvious to me how this works. Would it make sense to include a docstring for this parameter?

@sohailc
Copy link
Member Author

sohailc commented Aug 7, 2019

@jenshnielsen, I think I have addressed your comment. Can you merge this PR if there are no further issues?

@jenshnielsen jenshnielsen merged commit c71d17c into microsoft:master Aug 8, 2019
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.

None yet

2 participants