3rd-party code in DiffractionScreenView

[pixelzoom]

Related to #259 code review.

Review of the 'Diffraction' screen was not part of this review, since it's not included in 1.0. But this bit of odd code in DiffractionScreenView was noticed, and generated a discussion that's worth capturing here.

This was the code in questions. $h seemed like a very odd function name.

      const $h = function( k, l ) {
        if ( arguments.length === 0 ) {
          return h_hats;
        }

        const idx = k * dims[ 0 ] + l;
        return h_hats[ idx ];
      };

Slack discussion identified this as code that was copied from GitHub project https://github.com/turbomaze/JS-Fourier-Image-Analysis. Here's the Slack discussion:

Slack discussion

Chris Malley [4:19 PM]
I see this in DiffusionScreenView: const $h = function( k, l ) { is that legal?

Sam Reid [4:20 PM]
checking….

Chris Malley [4:20 PM]
And I know I’m not supposed to review diffusion.

Sam Reid [4:21 PM]
Are you asking if JS variable names can start with the $ character?

Chris Malley [4:21 PM]
Yes, I’ve never seen that. And why?

Sam Reid [4:22 PM]
variables that begin with $ are common when using jquery, see https://stackoverflow.com/questions/205853/why-would-a-javascript-variable-start-with-a-dollar-sign

This code was adapted from an example that uses an open-source fast fourier transform library.

Chris Malley [4:23 PM]
And your reason for using $h here?

Sam Reid [4:23 PM]

This code was adapted from an example that uses an open-source fast fourier transform library.

Chris Malley [4:23 PM]
Yes, I read that. Your reason for feeling that $h needed to be ported literally?

Sam Reid [4:24 PM]
I’m guessing the entire example was copied verbatim

Chris Malley [4:25 PM]
Where is the attribution for the code that you copied? (edited)

// Usage code from JS-Fourier-Image-Analysis/js/main.js ?

Assuming that’s https://github.com/turbomaze/JS-Fourier-Image-Analysis… Is the license (MIT) compatible?

Sam Reid [4:28 PM]
I’m seeing around 60 lines of code copied from https://github.com/turbomaze/JS-Fourier-Image-Analysis/blob/master/js/main.js

The delta in c98693b has REVIEW comments that are relevant:

      //REVIEW while $h may have been in the copied code, consider using a more 'conventional' function name.
      // store them in a nice function to match the math
      //REVIEW* I'm not too familiar with this code.  If it survives to dev testing, I would like to factor out
      //REVIEW* as much of this code to sherpa as I can.  For now, I renamed the function.
      const lookupHHat = ( k, l ) => {
        if ( arguments.length === 0 ) {
          return h_hats;
        }

        const idx = k * dims[ 0 ] + l;
        return h_hats[ idx ];
      };

So to summarize:

• A chunk of code was copied from JS-Fourier-Image-Analysis/js/main.js
• The code may or may not survive. If it does survive, the plan is to move it to sherpa.
• Compatibility of the license (MIT) needs to be confirmed.
• While this screen will not be visible in 1.0, you should determine whether this code will appear in the .html files (it looks like 'yes'), and whether doing so violates any licensing.

[samreid]

I stripped out DiffractionScreen so it no longer appears in builds. I also asked on the slack channel:

Sam Reid [10:56 AM]
Will Unused images file or Unused string: fail an RC or production deploy? It seems like just a warning for dev version.

This seems sufficient to defer for 2.0. @pixelzoom if you concur, please reassign to me and add "deferred" label.

[pixelzoom]

Looks like 2 images and 1 string, nothing of consequence for 1.0.

% grunt
...
>> require.js optimization for brand: phet complete (4996895 bytes)
>> Unused images file: WAVE_INTERFERENCE/airy-disk-10.png
>> Unused images file: WAVE_INTERFERENCE/diffraction_screen_icon.png
>> Unused string: key=WAVE_INTERFERENCE/screen.diffraction, value=Diffraction
...

Deferring for 2.0.

[samreid]

After #346 we are no longer using turbomaze, and imageprocessing-labs code has been moved to sherpa. Also the unused images have been removed. Closing.