phetsims / ph-scale

"pH Scale" is an educational simulation in HTML5, by PhET Interactive Simulations.
http://phet.colorado.edu/en/simulation/ph-scale
GNU General Public License v3.0
0 stars 8 forks source link

Molecule count for hydronium and hydroxide ions different at pH = 7 #225

Closed Nancy-Salpepi closed 3 years ago

Nancy-Salpepi commented 3 years ago

Test device iMac

Operating System 10.15.7

Browser Safari 14.1.1 (also seen on MacBook Air with M1 chip + chrome)

Problem description https://github.com/phetsims/qa/issues/669

After initially moving OH- slider to maximum quantity of 5.0 mol (pH = 15), the molecule counts for hydroxide and hydronium ions were not equal when pH was returned to 7 using hydroxide slider. This was only seen when slider was used and not when number adjuster in pH panel was manipulated. If the pH was manipulated using the hydroxide slider after the number adjusters were used, molecule counts were equal at pH = 7.

Steps to reproduce

  1. select My Solution screen
  2. Set toggle to quantity
  3. check molecule count checkbox
  4. Move OH- slider all the way up the scale to 5.0 mol
  5. move the OH- slider back down until the pH = 7.

Visuals

https://drive.google.com/file/d/1x5Nrnp-dSAalCpTGHzLpm6uX7MY_fCod/view?usp=sharing

Troubleshooting information: !!!!! DO NOT EDIT !!!!! Name: ‪pH Scale‬ URL: https://phet-dev.colorado.edu/html/ph-scale/1.5.0-rc.1/phet/ph-scale_all_phet.html Version: 1.5.0-rc.1 2021-07-13 18:33:46 UTC Features missing: touch User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/14.1.1 Safari/605.1.15 Language: en Window: 1409x778 Pixel Ratio: 1/1 WebGL: WebGL 1.0 GLSL: WebGL GLSL ES 1.0 (1.0) Vendor: WebKit (WebKit WebGL) Vertex: attribs: 16 varying: 31 uniform: 1024 Texture: size: 16384 imageUnits: 16 (vertex: 16, combined: 32) Max viewport: 8192x8192 OES_texture_float: true Dependencies JSON: {}

pixelzoom commented 3 years ago

Thanks @Nancy-Salpepi. Reproduced in 1.5.0-rc.1 and master.

I'll investigate, but here's my best guess. Because the slider is continuous, the pH is not exactly 7.00 in the model. It's probably something like 7.005, but we're only displaying 7.00.

Nancy-Salpepi commented 3 years ago

I thought it might be due to rounding, but since the molecule count was equal in all other instances, I thought it was odd for it to be different here.

pixelzoom commented 3 years ago

Yep, pH is not exactly 7.00 in the model.

For example, in this case pH=6.997477692908731. And you can see that the H3O+ and OH- sliders have different values.

screenshot_1064

If I fiddle with the OH- slider a bit, I can make the H30+ and OH- sliders have the same value, and the pH is still not 7.00, it's actually 6.999667798625693.

screenshot_1066

So there's a signicant problems here. We have a continous slider that has a precision that allows us to set pH values that can't be differentiated by 2 decimal place. I'm not sure how to resolve this. Maybe make the sliders snap to one decimal place for the coefficient?

@arouinfar thoughts?

pixelzoom commented 3 years ago

Notes to self: If we want to constrain the slider coefficients to 2 decimal places, then that can be done in either GraphIndicatorDragListener (in drag function) or in LogarithmicGraphNode (in yToValue function). The idea would be round any slider value to 2 non-zero decimal places. E.g. 0.0003456789 would become 0.00035

pixelzoom commented 3 years ago

Maybe make the sliders snap to one decimal place for the coefficient?

I went ahead and implemented this approach in the above commit. @arouinfar if you can think of any problems with this, please jump in asap.

@Nancy-Salpepi please verify this fix in master. Then assign back to me, so that I can patch the 1.5 branches.

Nancy-Salpepi commented 3 years ago

Still looks the same to me. Mole values are still different at pH = 7 and the "molecule count" values for the ions in the beaker are still not equal.

Screen Shot 2021-07-19 at 2 45 02 PM
pixelzoom commented 3 years ago

@Nancy-Salpepi I could easily reproduce the problem before, and I'm unable to now in master. I see from your screenshot that you're using phettest to test master. So I suspect that phettest may not have pulled the latest changes, or you're experiencing caching in your browser. If you think that may be the case and aren't familiar with those issues, ask one of the other QA team to fill you in. In the meantime, please give it a try in 1.6.0-dev.1. Thanks!

Nancy-Salpepi commented 3 years ago

Sorry that was my fault. I was not in incognito mode. I couldn't reproduce the problem in master or in 1.6.0-dev.1

pixelzoom commented 3 years ago

No, master is sufficient, thanks. I'll assign this back to me for patching in 1.5.

Note to self: While this feature is only used in ph-scale, I think I'll also patch ph-scale-basics, so that they continue to share the same code base. The sha is https://github.com/phetsims/ph-scale/commit/e7b9bf4c46997e7dadf2dec0ab7bd75318154686.

pixelzoom commented 3 years ago

Patched in 1.5 branches for ph-scale and ph-scale-basics. Ready for verification in next RC.

Nancy-Salpepi commented 3 years ago

I was unable to get different values when the toggle was switched to quantity. However, when it was toggled to concentration I was still able to get the values to be different at a pH=7.

Screen Shot 2021-07-26 at 9 06 34 AM
pixelzoom commented 3 years ago

For https://github.com/phetsims/qa/issues/677.

Reproduced in master and 1.5.0-rc.2.

pixelzoom commented 3 years ago

@Nancy-Salpepi's screen shot in https://github.com/phetsims/ph-scale/issues/225#issuecomment-886706291 shows OH- concentration of 9.9e-8. That's the case that I'm seeing too. The problem also occurs if you drag the H3O+ slider to 9.9e-8.

The fix that was applied for this issue was to constrain the coefficient "slider" values to 1 decima place. In this case, 9.9e-8 instead of 9.87856419106703e-8. And by running with ?log, I can see that is happening correctly:

value=9.87856419106703e-8 adjustedValue=9.9e-8 pH=6.9956351945975515

The problem is that this constraint is not sufficient to eliminate the problem in all cases. pH is computed from OH- concentration like this (in PHModel.js):

concentrationOHToPH( concentration ) {
   return 14 + log10( concentration )
}

And with 9.9e-8 as the OH- concentation, here's the result in the browser console:

> 14 + Math.log10( 9.9e-8 )
6.99563519459755

And that gets rounded to 7.00 when displayed.

So I'm not sure how to resolved this. Some options:

(1) Do nothing and live with it. Potentially confusing when pH is neutral (7.00) and the concentrations of H3O+ and OH- are different. (2) Add a decimal place to the pH meter. 6.99563519459755 will display as 6.996. This will require making the pH meter wider. And we should probably change for both the Micro and My Solution screens. (3) Display truncated (instead of rounded) pH in the meter. 6.99563519459755 will display as 6.99. This would solve the "neutral" case. But it creates a problem for all other cases, because rounding is generally preferrable to truncation. (4) ... something else?

I'm strongly in favor of (2). When we're dealing with small concentration values, more digits are clearly required for pH.

Assigning to @arouinfar because this is a design problem. The number of decimal places that were specified for the pH meter is not sufficient. Please review the above options. Do you have other ideas? How would you like to proceed?

pixelzoom commented 3 years ago

I've implemented option (2) in master. The pH meter used in the Micro and My Solution screens now shows 3 decimal places for pH, and is therefore slightly wider. The pH meter in the Macro screen is unchanged, and still shows 2 decimals places. Screenshots are shown below for the problem cases in the My Solution screen, where concentration of H3O+ or OH- is 9.9e-8.

@arouinfar please test-drive in master. Is this an acceptable solution? If so, assign back to me to cherry-pick into the 1.5 branches. If not, please suggest an alternative.

screenshot_1090 screenshot_1084
pixelzoom commented 3 years ago

The wider pH Meter causes the pH probe to move a bit to the right in the My Solutions screen. I verified that this does not cause the probe to overlap the other UI elements that are displayed in the beaker. See screenshot below.

screenshot_1087
pixelzoom commented 3 years ago

Discussed with @arouinfar on Zoom. Adding a decimal place to the pH meter trades one precision problem for another. Changing the pH spinner by 0.001 increments is tedious, and doesn't change the H3O+ and OH- values. And diluting a solution (e.g. Spit) in Micro screen also is problematic. So I'll revert that change.

The next thing I'll try is a new option. This problem occurs only when the value is concentration is 9.9e-8. So when that value occurs, snap to 1.0e-7. I'll see how ugly that gets, and (if it's OK) have @arouinfar review.

arouinfar commented 3 years ago

@Nancy-Salpepi this was a really great find and I think the change @pixelzoom made to constrain the values set by the graph indicators was necessary.

Adding a decimal place to the pH meter trade one precision problem for another. Changing the pH spinner by 0.001 increments is tedious, and doesn't change the H3O+ and OH- values. And diluting a solution (e.g. Spit) in Micro screen also is problematic. So I'll revert that change.

Here are some examples that demonstrate the dilution issue on the Micro screen.

pixelzoom commented 3 years ago

Workaround implemented in the above 3 commits, which I'll need to cherry-pick to 1.5 branches.

This workaround is needed only when the graph is set to Concentration. For Quantity, neutral pH corresponds to H3O+ and OH- quantities of 5.0e-8 mol. And 4.9e-8 has no similar problem, as it results in a pH of 6.99.

@arouinfar please review in master, and let me know if this looks OK.

arouinfar commented 3 years ago

Looks good in master @pixelzoom. I've created an issue to update the Teacher Tips (referenced above).

pixelzoom commented 3 years ago

Patched in ph-scale 1.5. Not necessary to patch in ph-scale-basics 1.5, because this feature (and code) does not appear in that sim.

Ready for verification in the next RC.

KatieWoe commented 3 years ago

This looks ok in rc.3