phetsims / gas-properties

"Gas Properties" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
6 stars 6 forks source link

Change pressure noise algorithm to be more realistic. #245

Closed pixelzoom closed 1 month ago

pixelzoom commented 1 month ago

Related to https://github.com/phetsims/gas-properties/issues/236 ...

@matthew-blackman will lead this. @pixelzoom will review.

pixelzoom commented 1 month ago

The implementation looks OK, straightforward.

What I have been concerned with is having to revisit the behavior, and the time that's going to take.

From model.md (which also needs to be updated):

On each time step, pressure is computed precisely as P = NkT/V. The pressure gauge is given a bit of "noise" to make it look more realistic. The noise is a function of pressure and temperature. More noise is added at lower pressures, but the noise is suppressed as temperature decreases. Noise is disabled when pressure is being held constant. See PressureGauge if you'd like more specifics. If desired, noise can be disabled via query parameter pressureNoise=false.

The new implementation of noise is based only on pressure. Noise is no longer suppressed as temperature increases. MAX_NOISE was deleted, which had been tuned by committe in https://github.com/phetsims/gas-properties/issues/92.

Is the new behavior of pressure noise significantly different? Is it acceptable to the design team? I can't tell. At the very least, we'll need sign-off from @arouinfar and @kathy-phet.

pixelzoom commented 1 month ago

Reviewed briefly with @arouinfar and here are the concerns that came up. @arouinfar please correct/augment.

matthew-blackman commented 1 month ago

@arouinfar and I had a follow-up discussion on the above points and came to the following agreements:

pixelzoom commented 1 month ago

@matthew-blackman @arouinfar Let's discuss this.

Problems:

(1) Was https://github.com/phetsims/gas-properties/commit/c77e004b4f498e7ddad4a9fad79edbd83eeecd36 tested? When I add particles to the container, the sim fails validation of pressureKilopascalsNoiseProperty immediately after a particle contacts a wall, because the temperature is negative. Adding this assertion indicates that noise is negative:

        noise = dotRandom.nextGaussian() * 100;
+       assert && assert( noise >= 0, `invalid noise: ${noise}` );

(2) Using dotRandom.nextGaussian is getting the next Gaussian-distributed random number from the same Random instance that is used throughout the sim for other randomness in the sim (e.g. particle positions) and common code. That will almost certainly affect the distribution that you're getting for the pressure model. Have you tested with CODAP to verify that you're getting the desired distribution? Should you be using a separate Random instance specifically for the noise?

(3) I don't understand why the amount of noise is now dotRandom.nextGaussian() * 100 kPA ("constant uncertainty model"), unaffected by pressure or temperature. If I add 1 particle to the container with noise off, the pressure is 12kPa. With noise on, the pressure will now vary from [12,120] kPA. That seems like way too much noise.

(4) model.md has not been updated, as noted in https://github.com/phetsims/gas-properties/issues/245#issuecomment-2120954347.

pixelzoom commented 1 month ago

Perhaps you're assuming that nextGaussian returns a value in the range [0,1]? That's not the case. Read doc and see browser console tests below.

> phet.dot.dotRandom.nextGaussian()
1.153788985390105
> phet.dot.dotRandom.nextGaussian()
0.453536062809
> phet.dot.dotRandom.nextGaussian()
-0.03803220771840494
pixelzoom commented 1 month ago

Also noting that gas-properties and gases-intro have been broken in CT since https://github.com/phetsims/gas-properties/commit/c77e004b4f498e7ddad4a9fad79edbd83eeecd36 was pushed.

matthew-blackman commented 1 month ago

https://github.com/phetsims/gas-properties/commit/c77e004b4f498e7ddad4a9fad79edbd83eeecd36 was tested and is exhibiting behavior consistent with the most recent design consensus. We can certainly have a broader discussion about the amount of noise and the overall design of the noise, but the possibility of a negative pressure reading with variability was agreed to be realistic for ultra-low pressure values.

There is also the issue that the sim is combining a micro view of the particles with a macro view of the sensor, which can sometimes make things look odd but I think overall works okay.

Would removing the validators for pressureKilopascalsProperty and pressureAtmospheresNoiseProperty be sufficient to handle the CT errors? I'm not seeing anywhere else that these values need to be positive.

pixelzoom commented 1 month ago

https://github.com/phetsims/gas-properties/commit/c77e004b4f498e7ddad4a9fad79edbd83eeecd36 was tested

I can't run the sim, and neither can CT.

but the possibility of a negative pressure reading with variability was agreed to be realistic for ultra-low pressure values.

Who agreed to this?

Would removing the validators for pressureKilopascalsProperty and pressureAtmospheresNoiseProperty be sufficient to handle the CT errors? I'm not seeing anywhere else that these values need to be positive.

No, that is not acceptable because:

  1. Pressure is not a negative quantity, regardless of whether it contains noise.
  2. GaugeNode (common code) will show the needle going below the 0 tick mark.
  3. The sim will display negative pressure values. If the goal here is to indeed be "realistic", that is impossible.

Please fix the algorithm so that pressure is not negative.

arouinfar commented 1 month ago

I haven't reviewed this behavior in detail, my discussion with @matthew-blackman in https://github.com/phetsims/gas-properties/issues/245#issuecomment-2123119656 was more about the Gaussian approach, in general, and removing the temperature dependence. We still need to decide on the Gaussian width and what to do about negative values. I agree with @pixelzoom -- negative pressures are problematic.

pixelzoom commented 1 month ago

This work has become too distuptive in main, blocking my progress. I've reverted the commits that have been made so far (linked above). Please continue your work in a branch.

matthew-blackman commented 1 month ago

@arouinfar @pixelzoom and I met and discussed the following:

Feel free to close this issue if that is all.

pixelzoom commented 1 month ago

Thanks for the nice summary @matthew-blackman.

Closing.