-
Notifications
You must be signed in to change notification settings - Fork 2
setSaveAfterEachStimulus() overwrites trials' existing on_finish callback #38
Comments
Aha! I think I need to |
Related to above, I'm now having a weird issue now with yalc and
I believe it's I also tried editing package.json in pushkin/front-end/.yalc/basicTest_web directly so that it points to pushkin-client in the parent folder instead of the disappearing .yalc folder, but prep seems to overwrite that, and I just get the same error again. |
That's correct. You don't want to put the dev version of pushkin-client into a template. Since you are installing from a template, it'll install using the published version. You then go in and switch it to the .yalc version.
I don't think so. I think that's made during
You are right. That won't work. Basically, when you run I'll see if there's a simple solution. Worst-case scenario, I think we can make |
OK, according to the yalc repository issues, this is an expected issue. What is going on is that our experiment templates specify which files to use when installing using the "files" option in package.json. It didn't say we should include ".yalc", so it didn't. What you need to do is update, for instance:
I checked, and the Anyway, making this changed resolved the earlier error, but now I'm getting a different one:
8 |
|
I was able to add ".yalc" to package.json.files and now So I guess I'll look back at what I added to |
Huh. |
Nevermind. It's working for me now. No idea why. @jessestorbeck -- can you please update the docs to correctly explain how to test local versions? Thanks. |
@jkhartshorne Can do. Will we push an update to the exp templates that adds .yalc to the files list? i.e. do I need to instruct people to make that change? |
No. There won't always be a .yalc, and it will crash if it's not there. Easier to add it manually when you want to do local testing. You'll of course need to undo it before publishing.
I just tried with the basic experiment template, and it still pushed to the database fine, even using the new version of pushkin-client. Not sure what that means. I'm currently using commit b065705d281a0ecaa2d6f69a0258ae71d3c98951 of the basic experiment template. |
Hmm I am using the latest basic template v5.0.1. I'm not sure why it didn't work with the published version, but I can try that version too (pushkin-consortium/pushkin-exptemplates-basic@b065705). The updated yalc instructions are now added to this PR: pushkin-consortium/pushkin#234. Did you try to add any on_finish callbacks to the experiment? This was the test code I was using:
|
I played around in console to see what code like this does:
I think it actually sets up an infinite loop with on_finish calling itself over and over. So try in the console:
The final line should give you:
I'm seeing if I can work out an alternative... |
So the reason on_finish functions aren't triggering doesn't have to do with saveAfterEachStimulus(). I actually ran your example here without saveAfterEachStimulus() invoked, and it still didn't trigger the on_finish function. I'll need to look at |
Nothing changed from the user side with how |
-----EDIT-----
|
I have confirmed, however, that with this upgrade no data is now written to the database at all. So I'm going to look at that next. |
OK, I think I figured it out. HOWEVER, with our new code, we simply list the function, but doesn't give it any arguments:
This can be solved with
This works for the example we've been working with. So the |
FYI it might be easier and less intrusive to the user's timeline to use the |
Relevant docs: https://www.jspsych.org/7.3/overview/events/#on_data_update |
@jodeleeuw Thanks for that. That does sound better. However, the new code is passing the tests. (I wrote it last night but hadn't finished writing the unit tests.) If it also works in end-to-end (which I don't have automated), I'll just stick with it for now. But I've added what you wrote as a comment in the relevant code in |
OK. Somehow it updated most of the I'm going to try @jodeleeuw's suggestion. |
I just wanted to confirm that this can only be modified during initJsPsych(). There's no good way to edit it afterwards, right? |
I just tried with pushkin client at commit e3f0e8f. I can confirm that all the data looks good except for the SPR template. I'm getting nothing from the SPR plugin (should appear as self-paced-reading), but I am getting data from the comprehension questions, which were missing previously. |
This isn't supported behavior, meaning it'll almost certainly break when we release v8.0, but you can do this: const jsPsych = initJsPsych()
jsPsych.opts.on_data_update = (data) => { console.log(data) } |
Yah. Same here. I'm rewriting to use @jodeleeuw's suggestion. It is much more elegant, assuming it works. (I'm having a little trouble getting the syntax right for use with |
What about just adding it directly to the template instead of making a separate api thing for it? |
Yah, I decided that was equally easy. So I did that. But I think the issue with not recording data from the actual SPR trials is a jsPsych thing -- or at least, our failure to understand jsPsych. So
The timeline can be found in
|
@jessestorbeck while we wait to see if @jodeleeuw can find something wrong with our code, do I remember correctly that there's a plain vanilla jsPsych version of this experiment? It presumably posts all the data at the end? Can you see if it posts the individual trial data? |
@jodeleeuw @jessestorbeck OK pretty sure it's an issue with the contributed plugin. jspsych/jspsych-contrib#75 @jessestorbeck -- do you want to try putting together a minimal working example that doesn't use pushkin but shows this same error? Or a minimal working example that doesn't use pushkin and doesn't show the error. Either one would be helpful! |
Here's what one run of the SPR procedure in vanilla jsPsych looks like:
We get this pre-SPR readiness check in Pushkin, but then the SPR plugin takes over, and the following data doesn't make it to the database.
Now the SPR plugin is done, and we do get the following trials in the database.
I am wondering if it has anything to do with the fact that the SPR plugin is a bit different in that the data it outputs looks like it's coming from distinct trials, but the |
How did you get that printed out? Can you please post all the code somewhere?
That seems quite plausible. |
@jkhartshorne the vanilla jsPsych code is now here. |
To add to that, the data I pasted in above is what comes out after you finish the timeline as a vanilla jsPsych experiment. It comes from
|
How do you test it? If I try opening the HTML file in my browser, I get: |
I use the Live Server extension for VSCode |
So looking at the SPR plugin, I see that after every word, it writes data using jsPsych.data.write()
The jsPsych docs say this is a bad idea:
In the example of the code, we are warned
|
@jessestorbeck I have rewritten the self-paced-reading plugin: https://github.com/jkhartshorne/jspsych-contrib/tree/SRD-data I'm not entirely sure how to test it locally. Your MWE pulls the code from an online library, and I assume we don't want to do that. @jodeleeuw may have some thoughts. Anyway, please test it out and see if you can get it to work. If it's easier, you can try it from within Pushkin using yalc to import the module locally. |
I couldn't figure out how to test it within a plain jsPsych experiment, but I was able to I noticed in the index.js for the reading template (pushkin-consortium/pushkin-exptemplates-reading@ebbd439), there's still a call to
which I replaced with:
But that didn't fix the missing data issue. |
The current version of the
setSaveAfterEachStimulus
looks like this:What seems to be happening is that any existing on_finish callback one includes in experiment.js gets overwritten here. The fix I tried to implement is this (see af3ab32):
I think I've followed the instructions here correctly to test my changes via yalc, but the data in the database look exactly the same as before. I'm not sure if my code does nothing or I didn't use yalc correctly. I think there may be some missing steps in the instructions, so here's what I did:
yarn install
andyarn build
in the root of pushkin-client when I tried this the first time, but it seems like that might be needed, so I did that. Then I ranyalc publish
pushkin/front-end
of my site folder, I ranyalc add pushkin-client
. I can see the local version of pushkin-client in the package.json now.From there, I was able to run
prep
andstart
, load the experiment, and inspect the data. So maybe my fix does nothing?The text was updated successfully, but these errors were encountered: