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

added option to display calibration plot within expServer #245

Closed
wants to merge 4 commits into from

Conversation

kevin-j-miller
Copy link
Contributor

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.

Copy link
Contributor

@k1o0 k1o0 left a 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');
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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 Show resolved Hide resolved
+srv/expServer.m Outdated Show resolved Hide resolved
+srv/expServer.m Outdated Show resolved Hide resolved
+srv/expServer.m Outdated
fig = figure; hold on
plot([calib.measuredDeliveries.durationSecs],...
[calib.measuredDeliveries.volumeMicroLitres],'x-')
if isfield(calib, 'dateTime')
Copy link
Contributor

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...

Copy link
Contributor Author

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.

Copy link
Contributor

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:

Rigbox/+hw/devices.m

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

@kevin-j-miller
Copy link
Contributor Author

kevin-j-miller commented Feb 5, 2020

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.

@k1o0
Copy link
Contributor

k1o0 commented Feb 5, 2020

@kevin-j-miller If everything looks good to you I'll merge.

@kevin-j-miller
Copy link
Contributor Author

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.

image

@k1o0
Copy link
Contributor

k1o0 commented Feb 7, 2020

Hmm, that's weird. The background doesn't go black on my rig. It should return to whatever value bgColour is set to in expServer. Changing the background isn't so important be we should figure out why it goes black for you. Are you calling expServer with any inputs?

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 hw.debugWindow.

@k1o0
Copy link
Contributor

k1o0 commented Feb 7, 2020

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:

rig = hw.devices([], false);
win = rig.stimWindow;
win.open();

sz = 20;
def = Screen('TextSize', win.PtbHandle, sz);

msg = sprintf('Default text size: %.0f,\n current size: %.0f', def, sz);
win.drawText(msg, 'centerblock', 'center', win.White, 1, 40)
win.flip()
pause(5)

Screen('TextSize', win.PtbHandle, def);
win.close;

@k1o0 k1o0 linked an issue May 3, 2020 that may be closed by this pull request
@k1o0 k1o0 added the invalid label May 3, 2020
@k1o0
Copy link
Contributor

k1o0 commented May 3, 2020

I'm going to merge this into the branch for the next release, so I'm closing this PR.

@k1o0 k1o0 closed this May 3, 2020
@k1o0 k1o0 deleted the dev_kevin branch February 10, 2021 11:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants