search

phetsims / natural-selection

"Natural Selection" is an educational simulation in HTML5, by PhET Interactive Simulations
GNU General Public License v3.0
3 stars 7 forks source link

recessiveMutants list did not support PhET-iO state #362

Open zepumph opened 2 months ago

zepumph commented 2 months ago

Discovered over in https://github.com/phetsims/natural-selection/issues/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 (https://github.com/phetsims/natural-selection/issues/361).

pixelzoom commented 2 months ago

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

zepumph commented 2 months ago

Oops. my mistake. recessiveMutants

pixelzoom commented 2 months ago

Changing title to indicate recessiveMutants.

pixelzoom commented 2 months ago

... 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 commented 2 months ago

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

https://github.com/phetsims/natural-selection/commit/fe0b0e47c8fd34d67ed1b7179e419866ae67385a

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

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

@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

Solutions

pixelzoom commented 2 months ago

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

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 https://github.com/phetsims/natural-selection/issues/362#issuecomment-2111262064 seem like the best options to consider.

pixelzoom commented 2 months ago

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

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 https://github.com/phetsims/natural-selection/issues/361#issuecomment-2118417824.

zepumph commented 2 months ago
pixelzoom commented 1 week ago

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

brent-phet commented 1 week ago

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

brent-phet commented 1 week ago

There is significant traffic outside of PhET to NS 1.5

image
zepumph commented 1 week ago

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

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 https://github.com/phetsims/natural-selection/issues/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 https://github.com/phetsims/natural-selection/issues/363. If you want to publish 1.6, then resolving that issue needs to be prioritized.