Support "maxConcentration not saturated" in the model

phetsims / molarity

"Molarity" is an educational simulation in HTML5, by PhET Interactive Simulations.

http://phet.colorado.edu/en/simulation/molarity

GNU General Public License v3.0

2 stars 6 forks source link

Support "maxConcentration not saturated" in the model #196

Closed zepumph closed 4 years ago

zepumph commented 4 years ago

From https://github.com/phetsims/molarity/issues/154, @emily-phet recommended proceeding with there being three states as it pertains to concentration:

At a concentration (between 0 and max)
At the max concentration but not saturated (because saturation is tied to the amount of precipitate in the solution)
Saturated, in which there is precipitate and it is still at max concentration.

To support this fully, and not just via ConcentrationDescriber, @twant and I came up with the following todo list:

[x] move ConcentrationDescriber.isSaturatedNoSolids to Solution.js
[x] Rename to atMaxConcentration
[x] Audit all usages of isSaturated to make sure it isn't actually saying atMaxConcentration()

zepumph commented 4 years ago

I implemented the above checkboxes above. I added one TODO to look into a usage of isSaturated.

// TODO: is this correct? https://github.com/phetsims/molarity/issues/196

Please note that I was not able to test this at all. I could not reproduce the case from https://github.com/phetsims/molarity/issues/154 even. When I did the exact same values, this time it showed that there was precipitate (even before the above commit). Will you please review and let me know what you think is happening. It is likely my fault and I just don't know how.

How would you like to handle the TODO? Does this really have to do with saturation? or should we change things to handle maxConcentration? Does a designer need to get involved?

Over to you to try to get this "max concentration not saturated" alert to fire, to review commits, and to review added TODO.

twant commented 4 years ago

@zepumph yes, I'm seeing that in the exact case we're talking about (where the concentration matches the max concentration and there are no solutes), the incorrect alerts are showing. I'll take a look now and see if I can figure out why!

twant commented 4 years ago

Found it! atMaxConcentration() should be comparing soluteProperty.value.saturatedConcentration with concentrationProperty, but it was comparing soluteAmountProperty.value.saturatedConcentration. Everything else looks good to me! @zepumph Pushing changes now, and then will assign over to you to review and close.

zepumph commented 4 years ago

Ahhh fooey. Good catch! Closing