recessiveMutants list did not support PhET-iO state

[zepumph]

Discovered over in #361, I believe that until recent changes in that issue which changed how state was supported, the list of recessiveBunnies was not correct when setting state. This is because it wasn't a stateful list, and it wasn't updated based on stateful notifications (like live/dead bunnies were). I'd like to discuss this with @pixelzoom because it will determine how I need to support migration for NS in that other issue (#361).

[pixelzoom]

I can't find anything named recessiveBunnies. What specifically are you referring to?

[zepumph]

Oops. my mistake. recessiveMutants

[pixelzoom]

Changing title to indicate recessiveMutants.

[pixelzoom]

... This is because it wasn't a stateful list, and it wasn't updated based on stateful notifications (like live/dead bunnies were)

I don't understand, please clarify. All of these arrays are instantiated (and instrumented) using the same createBunnyArray function. How could recessiveMutants not have been stateful?

In BunnyCollection.ts:

    this.liveBunnies = createBunnyArray( {
      tandem: tandem.createTandem( 'liveBunnies' ),
      countsPropertyFeatured: true
    } );

    this.deadBunnies = createBunnyArray( {
      tandem: tandem.createTandem( 'deadBunnies' )
    } );

    this.recessiveMutants = createBunnyArray( {
      tandem: tandem.createTandem( 'recessiveMutants' ),
      phetioDocumentation: 'for internal PhET use only'
    } );

[zepumph]

Yes, but those arrays were phetioState:false until last week.

fe0b0e4

It worked well for liveBunnies and deadBunnies since they were updated correctly based on bunny creation notifications, but recessiveMutants weren't updated correctly from new bunnies being created, so state wasn't capturing what bunnies had successfully reproduced via this list.

[pixelzoom]

Ah, ... I never realized that ObservableArray was phetioState: false, or that you had changed it to true.

I'm not clear on what needs to be done (if anything) or what's next.

Schedule a time with me if you'd like to discuss.

[zepumph]

@pixelzoom and I discussed this synchronously, and we determined that there doesn't seem to be an easy fix for patching the release branch. We don't want to change phetioState:true and change the API in the published version. It is up to managers as to how much time to devote to patching this issue.

The bug is that in many cases, resessiveMutants will not be set during state. This means that mateEagerly (a hollywooding feature to help produce recessive expressing bunnies quicker), will basically not do anything. To be clear, this is fixed on main. We may want to hold off on this until someone complains.

Problems

• The behavior of mating is substantially different from the original design after setting PhET-iO state on a sim. To varying degrees depending on what the list of recessiveMutants are when the state is set onto the sim. Recessive mutations are not going to appear in the population as quickly as they would without this bug.
• Likely causing a memory leak since this array isn't cleared on state setting, and is pointing to bunnies that don't exist from the newly set state.
• Could cause a runtime exception if trying to lookup one of the recessiveMutants in the bunny PhetioGroup when it doesn't exist in there.

Solutions

• Publish a minor version directly from the most recently published version, where all we do is change phetioState:true on the recessiveMutants.
• Ignore this until someone complains about it.
• Change the API of the currently published sim through a maintenance release (quick and dirty, not recommended)

[pixelzoom]

Problems

• The behavior of mating is substantially different from the original design after setting PhET-iO state on a sim. To varying degrees depending on what the list of recessiveMutants are when the state is set onto the sim. Recessive mutations are not going to appear in the population as quickly as they would without this bug.

FYI, here's the section in model.md that describes the function of the recessiveMutants array: https://github.com/phetsims/natural-selection/blob/main/doc/model.md#recessive-mutants. It is a feature that was added to the mating model to ensure that a recessive mutation appears in the phenotype as soon as possible. Without statefulness of the recessiveMutants list, the genetics of the population will be significantly different.

[pixelzoom]

The lastest release of Natural Selection is 1.5, published 8/17/23. It already was already converted to TypeScript, and supports dynamic locale. There would be no benefit (and much cost) to publishing off a new 1.6 release branch from main. So the "Solutions" @zepumph enumerated in #362 (comment) seem like the best options to consider.

[pixelzoom]

After some clarification from @zepumph in Slack, here's my opinion on the options:

• Publish a minor version directly from the most recently published version, where all we do is change phetioState:true on the recessiveMutants.

This involves creating a 1.6 branch off of the 1.5 branch. PhET creates release branches off of main, not other release branches. Because this deviates from the PhET process, I do not recommend this. If a 1.6 release is needed, it should be created off of main, with a full QA cycle.

• Ignore this until someone complains about it.

I don't recommend this. This is a serious problem.

• Change the API of the currently published sim through a maintenance release (quick and dirty, not recommended)

This would break the "do not change API for a minor release" policy, but seems better than the other proposals.

So imo, the right way to do this is in a new 1.6 release, off of main, with a full QA cycle.

[pixelzoom]

I'm wondering if another option would be to publish 1.5.7 (1.5.6 is the current published version) with a migration processor that fixes things. We can add migration processors for maintenance versions, correct? The migration processor is described in #361 (comment).

[zepumph]

• In addition to determining if we need to patch the old version of NS, we also need to decide how to support migration on main. See CT: Error: Assertion failed: counts out of sync #361 (comment), and note the TODO pointing to this issue in the NS migration processors.

[pixelzoom]

I've added this issue to the PHET-iO meeting agenda for 7/11/24.

[brent-phet]

I've added this as an Epic/Sub-epic for scheduling purposes.

[brent-phet]

There is significant traffic outside of PhET to NS 1.5

[zepumph]

Today during PhET-iO meeting, we decided that the best course of action is to publish 1.6 with the fix. This will allow a facility for migrating old, buggy states into the new version. This doesn't mean that this is top priority for an upcoming iteration, but we will work its way into planning.

When this is a priority, it would probably be good to have @pixelzoom and @zepumph review the migration rule for populating the recessiveMutatants to make sure it will well for current Standard Wrappers.

[pixelzoom]

Today during PhET-iO meeting, we decided that the best course of action is to publish 1.6 with the fix ...

@kathy-phet @brent-phet Note that Natural Selection has been broken with a scenery layout problem since at least 5/17/24, see #363. It's been very frustrating trying to get attention on that issue, so I've given up. I am adding @kathy-phet and @brent-phet to the assignees for #363. If you want to publish 1.6, then resolving that issue needs to be prioritized.