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

self-paced-reading doesn't record data #75

Closed
jkhartshorne opened this issue Aug 21, 2023 · 11 comments · Fixed by #79
Closed

self-paced-reading doesn't record data #75

jkhartshorne opened this issue Aug 21, 2023 · 11 comments · Fixed by #79

Comments

@jkhartshorne
Copy link

@jodeleeuw @igmmgi

I think that the self-paced reading plugin fails to trigger any of the events normally used to save data (such as on_trial_finish or on_data_update on any of the words except the last one.

You can find our timeline herehere.

Now consider:

    const jsPsych = initJsPsych({
      display_element: document.getElementById('jsPsychTarget'),
      on_finish: console.log(`I'm done`),
      on_data_update: function(data) {
        console.log('Just added new data. The contents of the data are: '+JSON.stringify(data));
      },
      on_trial_finish: function(data) {
        console.log('A trial just ended.');
        console.log(JSON.stringify(data));
      }
    });

Critically, console.log only triggers on the final word of a sentence:

Screen Shot 2023-08-21 at 11 51 29 AM

If I have time, I'll try to come up with a minimal working example.

@igmmgi
Copy link
Contributor

igmmgi commented Aug 21, 2023

Yes, a MWE would help. It has been a while since I worked on this code. But running my example does give me one data line per word in the sentence. Looking at the code, the end_trial function only triggers on the final word (as you observed), but the data to the earlier words in the sentence are saved via this.jsPsych.data.write. What is in the results file at the end?
on_finish: function () {
jsPsych.data.displayData('csv');
}

@jodeleeuw
Copy link
Member

Looking at the code, the end_trial function only triggers on the final word (as you observed), but the data to the earlier words in the sentence are saved via this.jsPsych.data.write.

I hadn't noticed this before. This isn't technically a supported use case for jsPsych.data.write() because under the current data model we expect there to be exactly one entry per trial. For other plugins that have similar kinds of data we typically store the data in arrays.

@jkhartshorne
Copy link
Author

I'm fairly confident that's the issue: pushkin-consortium/pushkin-client#38 (comment)

I was making a MWE for posterity. And I'm also working on a pull request that should fix this. Based on the docs, I was planning on using jsPsych.data.addProperties(). @jodeleeuw you wouldn't suggest that?

@jkhartshorne
Copy link
Author

OK, here's the MWE: https://github.com/jkhartshorne/jspsych-test-spr

The critical bit is

    const jsPsych = initJsPsych({
      on_finish: function() {
        jsPsych.data.displayData();
      },
      on_trial_finish: (data) => console.log(data)
    });

If you watch the console.log, you'll see that when on_trial_finish is triggered, the data object it is passed contains only the final word in the sentence.

@jodeleeuw
Copy link
Member

No, that will add the data to all trials. Mainly used for things like subject IDs, condition assignment, etc.

I'd recommend modifying the plugin to remove calls to jsPsych.data.write() and instead store the sequence of responses for each variable in arrays. These can be unnested fairly easily in R using tidyr::unnest.

I do think some of the changes we are making to version 8 may allow us to explore more flexible data models. @bjoluc it would be interesting to consider whether to add a method to write a row of data before a trial is finished.

@jkhartshorne
Copy link
Author

@jodeleeuw -- my turn to complain to you about cross-platform compatibility. Apparently it's not trivial to get canvas installed on an M1 chip.

As I learned here, users of M1s and M2 need to first run

brew install pkg-config cairo pango libpng jpeg giflib librsvg

It may or may not be necessary to also globally install node-gyp using your favorite package manager (yarn or npm, etc.). I did just to check that I could get it installed, so I don't know if it's necessary. I'll add that to the docs and give you a pull request.

@jkhartshorne
Copy link
Author

I'm not entirely sure how to test this locally, but the tests work. (Or at least they did after I rewrote them.)

#77

@bjoluc
Copy link
Member

bjoluc commented Aug 21, 2023

Apparently it's not trivial to get canvas installed on an M1 chip.

Sadly, it's not trivial to get canvas installed in many circumstances. Canvas is a dependency of @jspsych/config needed for the canvas plugin tests. Terribly wrong decision to include it with the other testing deps in hindsight – I didn't know it was such a trouble maker. We'll remove this in @jspsych/config v2. Until then, let me exclude it manually in this repo, so we don't need to add instructions for it.

it would be interesting to consider whether to add a method to write a row of data before a trial is finished.

@jodeleeuw Interesting idea, but would require some thought and might add a lot of complexity for an edge case (?) that can also be solved by plugin code. Not something I'd like to maintain, I think 🙃

@bjoluc
Copy link
Member

bjoluc commented Sep 4, 2023

Until then, let me exclude it manually in this repo, so we don't need to add instructions for it.

I overlooked that the self-paced reading plugin tests need canvas, so that's not an option. I think we should add a general note about canvas installation trouble to the readme linking to https://github.com/Automattic/node-canvas/wiki#installation-guides for all platforms.

@jkhartshorne
Copy link
Author

Or accept my pull request.

@bjoluc
Copy link
Member

bjoluc commented Sep 5, 2023

Or accept my pull request.

Yes I saw the PR, thanks. canvas installation issues are not restricted to arm64 chips though and only mentioning Homebrew discriminates Windows users, so I'd prefer referencing the official documentation for everyone. I'll add a note to the Readme soon – unless you'd like to update your PR of course 🙂

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 a pull request may close this issue.

4 participants