Mini code-review of BlackbodyBodyModel.js

[ariel-phet]

The file https://github.com/phetsims/blackbody-spectrum/blob/master/js/blackbody-spectrum/model/BlackbodyBodyModel.js has me asking a few questions, and I think would be good to discuss briefly as a group.

I feel like in general I should be able to read the equation being used to calculate something, and this equation in particular seems obfuscated:

    /**
     * Function that returns the spectral radiance at a given wavelength (in nm)
     * The units of spectral radiance are in megaWatts per meter^2 per micrometer
     * @public
     * @param {number} wavelength
     * @returns {number}
     */
    getSpectralRadianceAt: function(wavelength ) {
      var intensityRadiation;
      var prefactor;
      var exponentialTerm;
      if ( wavelength === 0 ) {
        // let's avoid division by zero.
        return 0;
      }
      prefactor = ( FIRST_RADIATION_CONSTANT / Math.pow( wavelength, 5 ) );
      exponentialTerm = 1 / ( Math.exp( SECOND_RADIATION_CONSTANT / ( wavelength * this.temperatureProperty.value ) ) - 1 );
      intensityRadiation = prefactor * exponentialTerm;
      return intensityRadiation;
    },

Additionally, the constants are "far away" from the equation. My understanding was that in general if a constant is used in several places, good to have it grouped at the top of the file, but in the case where the constant is only used once, putting it in line might be acceptable as well.

The constants are

  // constants
  var GRAPH_NUMBER_POINTS = 300; // number of points blackbody curve is evaluated at
  var FIRST_RADIATION_CONSTANT = 1.191042e-16; // is equal to 2 hc^2  in units of watts*m^2/steradian
  var SECOND_RADIATION_CONSTANT = 1.438770e7; // is equal to  hc/k  in units of nanometer-kelvin
  var WIEN_CONSTANT = 2.897773e-3; // is equal to b in units of meters-kelvin
  var STEFAN_BOLTZMANN_CONSTANT = 5.670373e-8; // is equal to sigma in units of watts/(m^2*K^4)
var POWER_EXPONENT = 0.7; // an exponent to calculate the renormalized temperature

I am just trying to understand best practices here, were the constants written out this way so the units were clear?

In addition, although the "sim units" are in microns, everything in this file is being done in nanometers. There may have been some reasons for this choice (and I think @veillette was following the original Flash code in many ways) but I would like to better understand if this is a good way to do things. I think the discussion would also be useful for younger developers as they dive more into sim work.

[samreid]

In https://github.com/phetsims/wave-interference/issues/77 the recommendation was to implement the model in the displayed units (a deviation from our prior recommendation to implement models in SI/MKS).

FIRST_RADIATION_CONSTANT and SECOND_RADIATION_CONSTANT are only used once--based on prior discussion, they can be declared with the constants or locally.

It would be nice to explain the physics in an equation (PDF or image), then to match the implementation to that. Can you refer to a page online with the corresponding equation?

Here's a pruned version of the given implementation (untested) that we can discuss:

getSpectralRadianceAt: function( wavelength ) {
  if ( wavelength === 0 ) {
    // let's avoid division by zero.
    return 0;
  }
  var A = 1.191042e-16; // is equal to 2 hc^2  in units of watts*m^2/steradian
  var B = 1.438770e7; // is equal to  hc/k  in units of nanometer-kelvin
  var temperature = this.temperatureProperty.value;
  return A / Math.pow( wavelength, 5 ) / ( Math.exp( B / ( wavelength * temperature ) ) - 1 );
}

[pixelzoom]

I agree that it's difficult to "see" what the general equation is in BlackbodyBodyModel.js. That could easily be addressed with a comment above the computation that shows the full equation. For example in hookes-law.Spring, the equation for potential energy is clearly documented above the derivation:

    // @public potential energy, E = ( k1 * x1 * x1 ) / 2
    this.potentialEnergyProperty = new DerivedProperty( [ this.springConstantProperty, this.displacementProperty ],
      function( springConstant, displacement ) {
        return ( springConstant * displacement * displacement ) / 2;
      }, {
        units: 'joules',
        phetioType: DerivedPropertyIO( NumberIO ),
        tandem: options.tandem.createTandem( 'potentialEnergyProperty' )
      } );

[samreid]

Assigned to @SaurabhTotey to review the preceding comments and discussion from July 19 dev meeting and update code accordingly.

[ariel-phet]

@arnabp can you confirm this all looks good? and close if so?

[arnabp]

Elaborated a couple of the function docs a little more. Everything else looks good, closing.