Closed oliver-phet closed 4 years ago
@DianaTavares You were the designer for this sim. Any comments?
Hi Oliver,
Thanks for giving me this feedback. The teacher is right. Its a differente in the sectral radiance in the HTML5 version and in the flash version:
and yes, it is because I use the model that I find in most of the lectures about blackbody that I reviewed:
and it has the sr^-1 unit. The units in the sim don't include it, then my suggestion is that the HTML5 model should be modified to match the flash. But I never teach this subject and I prefer the opinion of @kathy-phet or @ariel-phet about it.
I do think the teacher is correct. I think it would be good to see if Mike Dubson has 15 minutes to consult with @arnabp on the model here.
@arnab it seems that there is basically just a factor of pi missing, can you try making that change and verify between the HTML5 and flash version?
Doing some research into this, most definitions of Spectral Radiance included the factor of pi, which made it difficult to confirm where it was supposed to be taken out....until I found out that it was labeled under a different definition: "flux density".
Which I believe raises the question of whether the y-axis label should be changed to Flux Density instead of Spectral Radiance (the units of the label should remain the same since technically the old Spectral Radiance units had left out the steradian unit).
From status meeting: We definitely want to move forward with taking out the factor of steradians from blackbody spectrum. However, "flux density" is not a very friendly term. Some alternate names, found from the links below, include "power density" and "spectral power density". The wikipedia page for spectral flux density also states:
The terms irradiance, radiant exitance, radiant emittance, and radiosity are closely related to spectral flux density.
Wikipedia page for spectral flux density Physics in a Nutshell page for Plank's Law
@kathy-phet and @arouinfar talked to Mike Dubson about label nomenclature and we decided to move forward "Spectral Power Density". It was briefly reviewed to look good in the sim. I've implemented the changes to master, does this satisfy the core issue?
I believe I've spoken too soon about the issue being resolved, as there's now a new one. With the new calculation to spectral power density, the graph at the max temp of 11000 K now looks like this:
The scale max scale level on the y axis will likely now have to increase and there will need to be a decision on whether the y axis graph value should be kept as is or converted to scientific notation, as it now extends to 6 total digits. Assigning @DianaTavares for these design decisions
I don't think we need to change the values to scientific notation for the max values in the y-axis. I think that the numbers can be read very well the way they are.
But a new click zoom is needed in order to see all the bigger curves from 8800 to 11000K. I suggest adding on click to the zoom to increase the Max range the axis to 2100.
What do you think @ariel-phet ?
Some further notes to extend https://github.com/phetsims/blackbody-spectrum/issues/110#issuecomment-526362780
While Mike Dubson found "Spectral Power Density" to be the most precise term, it was not his preferred option. He would prefer a more succinct option such as "Intensity" or "Flux". In his opinion, these terms are sufficiently vague to be scientifically accurate, while the units clarify the meaning.
@kathy-phet and I prefer the more descriptive "Spectral Power Density" and worked with @arnabp to adjust font sizes to accommodate the somewhat longer string. The axis labels and values now are all size 22, which matches the size of the "Blackbody Temperature" string.
Assigning back to @arnabp to add another vertical click zoom to set max range on the vertical axis so it fits on scale - as suggested by @DianaTavares .
The scale factor on the y axis is 5, so the next natural step up from 700 is 3500. If I set the max at 2100 then going back down results in 420 instead of 700 (this can be modified if needed, but I think there's still something to be said about having only the top step scale differently than the rest of the y axis zoom levels). Here is what 11000 K looks like with 3500 scale:
I'm passing this off to @jbphet since I'm leaving.
Diana noted in slack that at 3500:
I am worry that is very small for some temperature values
I've committed the 3500 to master for now. A change to the maxVerticalZoom
constant in BlackbodyConstants.js
. If scaling does end up needing to changed to fit the max down to 2100, you'll find the verticalZoomFactor
option in ZoomableAxesView
and the method that gets called when that zooming occurs at line 398 in ZoomableAxesView
.
Good luck to everyone 👋
@DianaTavares @jbphet - Can we meet briefly on Tuesday to nail this down? I sent an invite.
Yes, I can. I just accept the invitation
@DianaTavares, @kathy-phet, and I met today and discussed the issue and made some changes to the min, max, and default vertical zoom settings. We noticed some "brittleness" in the code, which I will work to fix at some point, but for now I've committed the changes we made, and will start moving all related changes to the release branch, and will put out a maintenance release. @kathy-phet said that this is high priority due to the fact that it is essentially a "science bug".
IMPORTANT NOTE - the changes that have been made up to this point will cause all translations of the Y-axis label to be lost when the maintenance release is complete. This issue should be assigned to @oliver-phet once publication is complete so that he can let translators know that they will need to address this.
I've published an RC version and set up a ticket for QA to test it. @kathy-phet and @DianaTavares - you should also check it out and chime in on whether the behavior appears correct. You can find the RC at https://phet-dev.colorado.edu/html/blackbody-spectrum/1.0.4-rc.1/phet/blackbody-spectrum_all_phet.html,
I already play with the sim and looks very good!
I checked against a few points on the flash sim and the results seemed comparable to me. Closing for 1.0.4-rc.1
Reopening since there needs to be some followup on this. @oliver-phet - The fix has been made and the sim has been republished. Can you please: [ ] - Let the original reporter of this issue know about the update [ ] - Inform translators that the wording of the y-axis label has changed and will need to be retranslated
So that the translation issue is clear, here is a screenshot of the Chinese translation as it looks after this change was published (the new label, which is now untranslated, is circled in red):
Reply from user:
Great -- thanks for correcting that error -- the numbers all jive now, and I can see that the area under the curve is equal to the Intensity.