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

Preference test #91

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Conversation

Simon-Stone
Copy link
Contributor

@Simon-Stone Simon-Stone commented May 31, 2022

DISCLAIMER: I am by no means a JavaScript programmer and I have cannibalized various other pages to clobber the new page together. I am absolutely certain that there the code could use some refactoring and cleanup, but I unfortunately do not have access to any experienced JavaScript programmers. If someone would take the time to do a proper code review of this PR, I would be happy to make any required changes, though.

I have a particular use case that was not covered by the current framework:
I want to compare a list different stimuli amongst itself in pairwise fashion. There is no reference or baseline, I simply want to compare every stimulus against every other stimulus and ask the subject which one they prefer. I also want to run the experiment online, possibly using crowdsourced participants, which means I need a way to discern who actually completed the experiment.

This PR adds several things to support this use case:

  • a new page preference_test,
  • the option to generate a random subject ID every time results are written,
  • the option to display a confirmation code on the final popup page after the results are written.

All new features are documented in experimenter.md and participant.md.
Two new YAML files pref.yaml and pref_grouped.yaml are added to configs/ to illustrate the use of the new page and options.

Closes #88

@faroit
Copy link
Collaborator

faroit commented May 31, 2022

@nullpunktTUD this is awesome. Thanks! I won't have time to review this before end of June. There are some javascript linter that could detect some errors maybe run this before...

Copy link
Collaborator

@faroit faroit left a comment

Choose a reason for hiding this comment

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

As I said, I won't be able to provide a full review but this looks already quite good to me.
@nullpunktTUD is there things missing? Otherwise I would give this a test and if things work as advertised, we can merge this.

Please also see my minor comments

configs/pref.yaml Show resolved Hide resolved
configs/pref.yaml Show resolved Hide resolved
configs/pref.yaml Show resolved Hide resolved
this.filePlayer.genericAudioControl.addEventListener((function (_event) {
if (_event.name == this.pageConfig.mustPlayback) {
this.played[_event.index] = true;
if (this.played.every(element => element === true)){
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure, but I think the use of => is incorrect here...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (this.played.every(element => element === true)){
if (this.played.every((element) => element === true)){

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this code was written by me. I'm happy to make the change, if you want me to, though. Please confirm.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, go a head. I discovered this using the grunt package task. You can check yourself if this solves the error.

I can try to enable this on github action in the future

Copy link
Collaborator

Choose a reason for hiding this comment

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

@nullpunktTUD i realized this is ES6 syntax, are you able to find and alternative that works on older browsers?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok. I will look into this later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you find an alternative for this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Simon-Stone do you have an update on this? i'm not a js expert myself but maybe chatgpt could help here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have an update, no. I can try to petition our AI overlords, though, and report back

@Simon-Stone
Copy link
Contributor Author

As I said, I won't be able to provide a full review but this looks already quite good to me. @nullpunktTUD is there things missing? Otherwise I would give this a test and if things work as advertised, we can merge this.

Please also see my minor comments

Thank you for taking the time to consider the PR!

I don't think there is anything missing and it seems to be working just fine.

I'll try to resolve all your minor comments soon.

@faroit faroit mentioned this pull request Jul 6, 2022
Co-authored-by: Fabian-Robert Stöter <faroit@users.noreply.github.com>
@Simon-Stone
Copy link
Contributor Author

Are there any changes left for me to make?

@faroit
Copy link
Collaborator

faroit commented Aug 2, 2022

Will look at this again end of the week

@JRMeyer
Copy link

JRMeyer commented Aug 9, 2022

this would be a super useful PR 😄

@nullpunktTUD Looks like you should add the pref test to configs/complete.yaml as well

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.

Paired comparison without a reference
3 participants