Skip to content

Check reward valve calibration#201

Merged
jkbhagatio merged 1 commit intodevfrom
checkRewardValveCalibration
Oct 30, 2019
Merged

Check reward valve calibration#201
jkbhagatio merged 1 commit intodevfrom
checkRewardValveCalibration

Conversation

@jkbhagatio
Copy link
Member

No description provided.

@jkbhagatio jkbhagatio changed the base branch from master to dev September 26, 2019 16:42
@jkbhagatio
Copy link
Member Author

@k1o0 the tests fail currently because they rely on generating a rig struct from hw.devices. Have you found a suitable way to create a mock rig struct? If so, please feel free to fix these tests, and add any else you think necessary, thanks

@kevin-j-miller
Copy link
Contributor

Looks reasonable to me! I think this is a great tool that'll be really helpful. Thanks for writing it!

One suggestion: Currently the code has a default daq channel ("ao0") then checks that this has a RewardValveControl signal generator in the hardware config. Maybe a better default would be to look through the config for the RewardValveControl and to calibrate it regardless of its daq port? (throwing an error if it doesn't find one, or finds more than one). This would let the code work argument-free if the valve wasn't on ao0, and might let you get away with eliminating the 'chan' argument entirely.

@k1o0
Copy link
Contributor

k1o0 commented Sep 27, 2019

I'll take a look at this over the weekend...

assert(~mod(length(varargin),2), 'Rigbox:hw:calibrate:partialPVpair', ...
'Incorrect number of Name-Value pairs')
if ~all(cellfun(@ischar, (varargin(1:2:end)))) || mod(length(varargin),2)
error('Rigbox:hw:calibrate:nameValueMismatch', ...
Copy link
Contributor

Choose a reason for hiding this comment

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

I chose the id partialPVpair because that's exactly the one MATLAB used in a builtin function. Whether they themselves have standardized all their IDs moot (they probably don't).

Copy link
Member Author

@jkbhagatio jkbhagatio Sep 30, 2019

Choose a reason for hiding this comment

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

Oh I see. I use nameValueMismatch in my own code. That's fine, feel free to make any changes you see fit before merging (rebasing onto dev and squash-merging)

@k1o0
Copy link
Contributor

k1o0 commented Sep 30, 2019

@k1o0 the tests fail currently because they rely on generating a rig struct from hw.devices. Have you found a suitable way to create a mock rig struct? If so, please feel free to fix these tests, and add any else you think necessary, thanks

Take a look at the test for calibrate.

@jkbhagatio
Copy link
Member Author

jkbhagatio commented Sep 30, 2019

Looks reasonable to me! I think this is a great tool that'll be really helpful. Thanks for writing it!

One suggestion: Currently the code has a default daq channel ("ao0") then checks that this has a RewardValveControl signal generator in the hardware config. Maybe a better default would be to look through the config for the RewardValveControl and to calibrate it regardless of its daq port? (throwing an error if it doesn't find one, or finds more than one). This would let the code work argument-free if the valve wasn't on ao0, and might let you get away with eliminating the 'chan' argument entirely.

I had it set this way in case there is more than one reward valve on a rig (e.g. your setup), so that the user can specify which valve they'd like to check.

It could be changed though so that if no channel name input arg is given, then it looks to see which channel is using a RewardValveControl class?

updated error message

bug fix checking signalGen class

made 'checkRewardValveCalibration' more general - have to test

optimized logical indexing

updated before creating mock objects for tests

added todo for creating appropriate tests

added test file
@jkbhagatio jkbhagatio force-pushed the checkRewardValveCalibration branch from c3002a6 to ce5feae Compare October 30, 2019 22:38
@jkbhagatio jkbhagatio merged commit 3d29c7e into dev Oct 30, 2019
k1o0 added a commit that referenced this pull request Nov 2, 2019
k1o0 pushed a commit that referenced this pull request Jan 3, 2021
updated error message

bug fix checking signalGen class

made 'checkRewardValveCalibration' more general - have to test

optimized logical indexing

updated before creating mock objects for tests

added todo for creating appropriate tests

added test file
k1o0 added a commit that referenced this pull request Jan 3, 2021
k1o0 pushed a commit that referenced this pull request Jan 4, 2021
updated error message

bug fix checking signalGen class

made 'checkRewardValveCalibration' more general - have to test

optimized logical indexing

updated before creating mock objects for tests

added todo for creating appropriate tests

added test file
k1o0 added a commit that referenced this pull request Jan 4, 2021
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.

4 participants