phetsims / projectile-data-lab

"Projectile Data Lab" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
0 stars 0 forks source link

CT: sampleMeanProperty should not be null in showingCompleteSampleWithoutMean phase #329

Open pixelzoom opened 1 month ago

pixelzoom commented 1 month ago

In both PDL and PSD:

projectile-data-lab : phet-io-state-fuzz : unbuilt
http://128.138.93.172/continuous-testing/ct-snapshots/1715368625981/phet-io-wrappers/state/?sim=projectile-data-lab&locales=*&phetioWrapperDebug=true&fuzz&phetioDebug=true&wrapperContinuousTest=%7B%22test%22%3A%5B%22projectile-data-lab%22%2C%22phet-io-state-fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1715368625981%22%2C%22timestamp%22%3A1715369403666%7D
Uncaught Error: Assertion failed: sampleMeanProperty should not be null in showingCompleteSampleWithoutMean phase. Projectiles in selected sample: 0. Sample size: 2
Error: Assertion failed: sampleMeanProperty should not be null in showingCompleteSampleWithoutMean phase. Projectiles in selected sample: 0. Sample size: 2
at window.assertions.assertFunction (http://128.138.93.172/continuous-testing/ct-snapshots/1715368625981/assert/js/assert.js:28:13)
at assert (SamplingField.ts:377:18)
at step (SamplingModel.ts:282:29)
at step (Sim.ts:432:21)
at apply (PhetioAction.ts:162:16)
at execute (Sim.ts:1048:30)
at stepSimulation (Sim.ts:1038:11)
at stepOneFrame (Sim.ts:1013:11)
[URL] http://128.138.93.172/continuous-testing/aqua/html/wrapper-test.html?url=..%2F..%2Fct-snapshots%2F1715368625981%2Fphet-io-wrappers%2Fstate%2F%3Fsim%3Dprojectile-data-lab%26locales%3D*%26phetioWrapperDebug%3Dtrue%26fuzz%26phetioDebug%3Dtrue&testInfo=%7B%22test%22%3A%5B%22projectile-data-lab%22%2C%22phet-io-state-fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1715368625981%22%2C%22timestamp%22%3A1715369403666%7D
[NAVIGATED] http://128.138.93.172/continuous-testing/aqua/html/wrapper-test.html?url=..%2F..%2Fct-snapshots%2F1715368625981%2Fphet-io-wrappers%2Fstate%2F%3Fsim%3Dprojectile-data-lab%26locales%3D*%26phetioWrapperDebug%3Dtrue%26fuzz%26phetioDebug%3Dtrue&testInfo=%7B%22test%22%3A%5B%22projectile-data-lab%22%2C%22phet-io-state-fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1715368625981%22%2C%22timestamp%22%3A1715369403666%7D
[ATTACHED]
[NAVIGATED] about:blank
[NAVIGATED] http://128.138.93.172/continuous-testing/ct-snapshots/1715368625981/phet-io-wrappers/state/?sim=projectile-data-lab&locales=*&phetioWrapperDebug=true&fuzz&phetioDebug=true&wrapperContinuousTest=%7B%22test%22%3A%5B%22projectile-data-lab%22%2C%22phet-io-state-fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1715368625981%22%2C%22timestamp%22%3A1715369403666%7D
[ATTACHED]
[NAVIGATED] about:blank
[ATTACHED]
[NAVIGATED] about:blank
[CONSOLE] enabling assert
[NAVIGATED] http://128.138.93.172/continuous-testing/ct-snapshots/1715368625981/projectile-data-lab/projectile-data-lab_en.html?brand=phet-io&ea&postMessageOnError&sim=projectile-data-lab&locales=*&phetioWrapperDebug=true&fuzz&phetioDebug=true&wrapperContinuousTest=%7B%22test%22%3A%5B%22projectile-data-lab%22%2C%22phet-io-state-fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1715368625981%22%2C%22timestamp%22%3A1715369403666%7D&frameTitle=source
[NAVIGATED] http://128.138.93.172/continuous-testing/ct-snapshots/1715368625981/projectile-data-lab/projectile-data-lab_en.html?brand=phet-io&ea&postMessageOnError&sim=projectile-data-lab&locales=*&phetioWrapperDebug=true&fuzz&phetioDebug=true&wrapperContinuousTest=%7B%22test%22%3A%5B%22projectile-data-lab%22%2C%22phet-io-state-fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1715368625981%22%2C%22timestamp%22%3A1715369403666%7D&frameTitle=destination
[CONSOLE] enabling assert
[CONSOLE] enabling assert
[CONSOLE] continuous-test-wrapper-load
[CONSOLE] continuous-test-wrapper-load
[CONSOLE] Assertion failed: sampleMeanProperty should not be null in showingCompleteSampleWithoutMean phase. Projectiles in selected sample: 0. Sample size: 2
[PAGE ERROR] Error: Error: Assertion failed: sampleMeanProperty should not be null in showingCompleteSampleWithoutMean phase. Projectiles in selected sample: 0. Sample size: 2
at window.assertions.assertFunction (http://128.138.93.172/continuous-testing/ct-snapshots/1715368625981/assert/js/assert.js:28:13)
at SamplingField.step (http://128.138.93.172/continuous-testing/ct-snapshots/1715368625981/chipper/dist/js/projectile-data-lab/js/sampling/model/SamplingField.js:301:19)
at SamplingModel.step (http://128.138.93.172/continuous-testing/ct-snapshots/1715368625981/chipper/dist/js/projectile-data-lab/js/sampling/model/SamplingModel.js:219:30)
at http://128.138.93.172/continuous-testing/ct-snapshots/1715368625981/chipper/dist/js/joist/js/Sim.js:353:22
at PhetioAction.execute (http://128.138.93.172/continuous-testing/ct-snapshots/1715368625981/chipper/dist/js/tandem/js/PhetioAction.js:137:17)
at Sim.stepSimulation (http://128.138.93.172/continuous-testing/ct-snapshots/1715368625981/chipper/dist/js/joist/js/Sim.js:884:31)
at Sim.stepOneFrame (http://128.138.93.172/continuous-testing/ct-snapshots/1715368625981/chipper/dist/js/joist/js/Sim.js:874:12)
at Sim.runAnimationLoop (http://128.138.93.172/continuous-testing/ct-snapshots/1715368625981/chipper/dist/js/joist/js/Sim.js:852:12)
[PAGE ERROR] Error: Error: Assertion failed: sampleMeanProperty should not be null in showingCompleteSampleWithoutMean phase. Projectiles in selected sample: 0. Sample size: 2
at window.assertions.assertFunction (http://128.138.93.172/continuous-testing/ct-snapshots/1715368625981/assert/js/assert.js:28:13)
at SamplingField.step (http://128.138.93.172/continuous-testing/ct-snapshots/1715368625981/chipper/dist/js/projectile-data-lab/js/sampling/model/SamplingField.js:301:19)
at SamplingModel.step (http://128.138.93.172/continuous-testing/ct-snapshots/1715368625981/chipper/dist/js/projectile-data-lab/js/sampling/model/SamplingModel.js:219:30)
at http://128.138.93.172/continuous-testing/ct-snapshots/1715368625981/chipper/dist/js/joist/js/Sim.js:353:22
at PhetioAction.execute (http://128.138.93.172/continuous-testing/ct-snapshots/1715368625981/chipper/dist/js/tandem/js/PhetioAction.js:137:17)
at Sim.stepSimulation (http://128.138.93.172/continuous-testing/ct-snapshots/1715368625981/chipper/dist/js/joist/js/Sim.js:884:31)
at Sim.stepOneFrame (http://128.138.93.172/continuous-testing/ct-snapshots/1715368625981/chipper/dist/js/joist/js/Sim.js:874:12)
at Sim.runAnimationLoop (http://128.138.93.172/continuous-testing/ct-snapshots/1715368625981/chipper/dist/js/joist/js/Sim.js:852:12)
[CONSOLE] continuous-test-wrapper-error
[CONSOLE] continuous-test-wrapper-error

id: "Sparky Node Puppeteer"
Snapshot from 5/10/2024, 1:17:05 PM
samreid commented 1 month ago

Experimental patch:

```diff Subject: [PATCH] Only consider landeded projectiles for determining if a sample is complete, see https://github.com/phetsims/projectile-data-lab/issues/329 --- Index: js/sampling/model/SamplingField.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/sampling/model/SamplingField.ts b/js/sampling/model/SamplingField.ts --- a/js/sampling/model/SamplingField.ts (revision 4d9eec05c31fd71b49a92004e4f6cc5e2c344b66) +++ b/js/sampling/model/SamplingField.ts (date 1715707819562) @@ -177,6 +177,10 @@ this.updateComputedProperties(); } ); + this.projectileLandedEmitter.addListener( () => { + this.updateComputedProperties(); + } ); + Tandem.PHET_IO_ENABLED && phet.phetio.phetioEngine.phetioStateEngine.stateSetEmitter.addListener( () => { this.updateComputedProperties(); } ); @@ -210,7 +214,7 @@ this.numberOfStartedSamplesProperty.value - 1; // Update the sample mean - const projectilesInSelectedSample = this.getProjectilesInSelectedSample(); + const projectilesInSelectedSample = this.getLandedProjectilesInSelectedSample(); // This multilink is called during transient intermediate phases, so we must guard and make sure we truly have a complete sample const isComplete = this.phaseProperty.value === 'showingCompleteSampleWithMean' && projectilesInSelectedSample.length === this.sampleSize;
samreid commented 1 month ago

This is pretty rare on CT. Showing once in the last 20x2 = 40 columns (2x multiplier is for 2x sims). @matthew-blackman and I investigated this on 3 occasions and do not have any significant leads. We do not believe the problem is likely to happen on a client machine, since nothing related to this was discovered during the QA process. We are also skeptical about a significant change to the architecture or logic, as it would risk breaking other behavior and warrant QA retesting. We last discussed that we should invite a consultant (perhaps @zepumph ?) to help us look at it with a fresh perspective. @matthew-blackman and @zepumph how should we proceed?

zepumph commented 1 month ago

I don't know the issue and I don't know how to reproduce it, but I have some thoughts!

In SamplingField, you have multiple Properties that aren't stateful, one of which is sampleMeanProperty. So at a general level this assertion doesn't surprise me at all, since I don't see any guarantee that sampleMeanProperty looks or behaves appropriately after a state set. I also see that numberOfStartedSamplesProperty is also unstateful.

I can only assume that this was purposeful for some good reason, so assuming that, would you like to try out recomputing these non stateful Property values from whatever stateful data we after upon set state completion? My first pass would be this line, but perhaps it will be buggy because it also will change numberOfCompletedSamplesProperty, which is stateful so perhaps shouldn't be changed.

Subject: [PATCH] blarg
---
Index: js/sampling/model/SamplingField.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/sampling/model/SamplingField.ts b/js/sampling/model/SamplingField.ts
--- a/js/sampling/model/SamplingField.ts    (revision 7a9b72093c43407b527510b427db662700409f45)
+++ b/js/sampling/model/SamplingField.ts    (date 1715976241831)
@@ -186,6 +186,10 @@
         this.isContinuousLaunchingProperty.value = false;
       }
     } );
+
+    Tandem.PHET_IO_ENABLED && phet.phetio.phetioEngine.phetioStateEngine.stateSetEmitter.addListener( () => {
+      this.updateComputedProperties();
+    } );
   }

   /**

Also, I'd like to note that wiring assertions into the stateSetEmitter is @pixelzoom and my favorite new state-testing tool. For example here.

https://github.com/phetsims/natural-selection/blob/fe0b0e47c8fd34d67ed1b7179e419866ae67385a/js/common/model/BunnyCollection.ts#L222-L226

Let me know if I can assist any further.