-
Notifications
You must be signed in to change notification settings - Fork 16
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
added option to display calibration plot within expServer #245
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea! I'll add a docstring, test and deal with missing calibrations
+srv/expServer.m
Outdated
@@ -39,6 +39,7 @@ function expServer(useTimelineOverride, bgColour) | |||
rewardToggleKey = KbName('w'); | |||
rewardPulseKey = KbName('space'); | |||
rewardCalibrationKey = KbName('m'); | |||
viewCalibrationKey = KbName('k'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about changing this to a 'v' and including the gamma calibrations in the display?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems reasonable to me! I think the water calibration is the thing the techs look at the most, but making more information more accessible seems like a good thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, I'll change it to 'v' and make an issue about adding other plots in the future.
+srv/expServer.m
Outdated
fig = figure; hold on | ||
plot([calib.measuredDeliveries.durationSecs],... | ||
[calib.measuredDeliveries.volumeMicroLitres],'x-') | ||
if isfield(calib, 'dateTime') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a case where dateTime
isn't a field? It's assumed to be there in other parts of the code...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was coming up for me, but I think it's an issue that's specific to some things I had done on my branch that re fixed now. I think in dev it's safe to assume dateTime will always be there if a calibration in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I removed the if statement because on master there would be an error thrown beforehand, here:
Lines 54 to 57 in 7d6b601
if isprop(sg,'Calibrations') | |
[newestDate, ~] = max([sg.Calibrations.dateTime]); | |
fprintf('\nApplying reward calibration performed on %s\n', datestr(newestDate)); | |
end |
This looks great! I love the idea of displaying the keyboard shortcuts too. I'd been thinking of printing out cheat-sheets and leaving them in the room, but this is much more elegant. |
@kevin-j-miller If everything looks good to you I'll merge. |
I just took a look using a rig. Two things weren't perfect: After displaying the calibration plot, the background went to black rather than grey, and also the help text was kind of hard to read (see below) The first I think could be solved by just not changing the background color to begin with (lines 420 and 423) -- the plot displays as an image, so it's background will still be white, just not the psychtoolbox background behind it. The second I think just needs a smaller fontsize or larger line spacing. |
Hmm, that's weird. The background doesn't go black on my rig. It should return to whatever value Regarding the text, it looks fine for me so I guess this depends on the resolution of the screen. Perhaps I should scale the font size by the vertical resolution? By the way for testing I use |
I looked into the text size setting. It seems PTB uses the system default, which should work fine. The clipping your seeing is not affected by the line spacing. In Windows Settings > System, is Scaling set to 100? Try running this code and telling me how it looks:
|
I'm going to merge this into the branch for the next release, so I'm closing this PR. |
I've had this in my branch for a while, and Laura requested that I try and merge it into the regular branch. If you press 'k' in expServer, a plot will appear that shows the rigs current calibration curve and the date of last calibration. I tweaked this to work with the regular rig, and tested it with zredone.