jspsych / jsPsych

Create behavioral experiments in a browser using JavaScript
http://www.jspsych.org
MIT License
1.03k stars 665 forks source link

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

Open ChristopheBossens opened 1 year ago

ChristopheBossens commented 1 year ago

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 commented 1 year ago

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

bjoluc commented 1 year ago

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 commented 1 year ago

@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_questionsand 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 commented 1 year ago

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 commented 12 months ago

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.