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

jsPsych.randomization.shuffle does not return a deep copy #2808

Open
ChristopheBossens opened this issue Oct 11, 2022 · 5 comments · May be fixed by #3128
Open

jsPsych.randomization.shuffle does not return a deep copy #2808

ChristopheBossens opened this issue Oct 11, 2022 · 5 comments · May be fixed by #3128

Comments

@ChristopheBossens
Copy link

While answering the question #2794 in the discussion forum, I noticed that the jsPsych.randomization.shuffle function does not create a deep copy of the array that is passed. For example

// Define a set of questions
    let survey_questions = [
        {
            'prompt' : 'How many cents is each coin worth?',
            'name'   : 'coinWorth',
            'options': ['10','1','0.1','0.01']
        },
        {
            'prompt'  : 'How many cents is each bill worth?',
            'name'    : 'billWorth',
            'options': ['10','1','0.1','0.01']
        },
        {
            'prompt' : 'How many cents is each question worth?',
            'name'   : 'questionWorth',
            'options': ['10','1','0.1','0.01']
        }
    ]

let survey_1_questions = jsPsych.randomization.shuffle(survey_questions);

Would shuffle the array, but using

survey_1_questions[0]['prompt'] = 'test';

affects the item in both arrays.

Is this something that should be fixed?

@jodeleeuw
Copy link
Member

Thanks @ChristopheBossens, I think we should fix this with 8.0. @bjoluc what do you think?

bjoluc added a commit that referenced this issue Oct 11, 2022
@bjoluc
Copy link
Member

bjoluc commented Oct 11, 2022

Thanks @ChristopheBossens! I added c9326e3 after reading the issue for the first time, but I misread the issue: I thought maybe the original array would be modified by jsPsych.randomization.shuffle, but it's not.

On a second read, I understand the issue now and I think I have to disagree: Shuffle does just what it says it does – it "Returns an array with the same elements as the input array in a random order". Why should it create a deep copy?

@ChristopheBossens
Copy link
Author

@bjoluc In this specific case we had an array of dictionaries that needed to be shuffled, and in the shuffled array the values in some of these dictionaries needed to be updated. Without a deep copy, changing a dictionary in the shuffled array also updates that same dictionary in the original array.

So the definition is correct in the sense (they are the same elements), but without a deep copy they are literally the same elements when dealing with arrays/dictionaries. I was wondering if that is what is intended? In the discussion above, we handled this by adding the following code before shuffling:

  // Deep copy and shuffle question order
    let survey_1_questions = JSON.parse(JSON.stringify(survey_questions));
    survey_1_questions = jsPsych.randomization.shuffle(survey_1_questions);

With this code, we can write

survey_1_questions[0]['prompt'] = 'test';

and this will only update that element in survey_1_questions and not also in survey_questions.

So the question is basically if this is something that should be handled by the framework, or perhaps add a disclaimer that the shuffling algorithm will not create a deep copy (and how to deal with that situation if you do need a deep copy)?

@bjoluc
Copy link
Member

bjoluc commented Oct 12, 2022

So the question is basically if this is something that should be handled by the framework, or perhaps add a disclaimer that the shuffling algorithm will not create a deep copy (and how to deal with that situation if you do need a deep copy)?

There are situations where deepCopying isn't required, explicitly not wanted (e.g., when object references matter), or doesn't work (like functions or OOP-style objects), so I don't think we should do it by default. But adding a hint about this sounds like a very good idea. Also, there's an internal deepCopy function already, maybe we should expose it? @jodeleeuw WDYT?

@bjoluc
Copy link
Member

bjoluc commented Sep 12, 2023

Turns out jsPsych.utils.deepCopy() is already available, but not listed in the docs, so closing this issue boils down to add some docs and reference them in the randomazation docs.

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.

3 participants