Rename currentAmplitudeProperty

[samreid]

During code review #103 we observed this code in Coil.ts:

export default class Coil extends PhetioObject {

  // This is a quantity that PhET made up. It is a percentage [-1,1] that describes the amount of current relative to
  // some maximum current in the model, and the sign indicates the direction of that current. View components can use
  // this value to determine how they should behave -- eg, how far to move a voltmeter needle, how bright to make a
  // light bulb, and how fast to move electrons.
  public readonly currentAmplitudeProperty: TReadOnlyProperty<number>;

Using a name like "amplitude" suggests that it is a non-negative number. So we recommend coming up with a new name that more closely reflects the documentation and behavior.

We also recommend adjusting the terminology "percentage" if the values like between -1 and 1.

[pixelzoom]

Adding @arouinfar to this, because this would be a huge change. It's the foundation of the Hollywood model, appears in model.md and in the PhET-iO API.

[pixelzoom]

@samreid said:

We also recommend adjusting the terminology "percentage" if the values like between -1 and 1.

Can you help me grok the typo in this sentence?

Can you point me to a case where we've used [-1,1] as the range for a percentage? Values expressed as percentages are in the range [0,100] throughout this sim. That range was used rather than [0,1] to accommodate PhET-iO, so that Property values correspond to the % values shown in the UI.

[samreid]

The documentation, referenced in the top comment, says:

It is a percentage [-1,1]

Normally a percentage goes from 0% to 100%, so perhaps "percentage" is inaccurate in this context. Perhaps my typo was to say "like" instead of "are".

[pixelzoom]

Documentation revised in Coil.ts and model.md, which purges the word "percentage" and hopefully clarifies the semantics of the value. Now we need to decide whether we need to change the name.

  // This is a quantity that PhET made up. It is a value in the range [-1,1]. The magnitude describes the amount of
  // current relative to some maximum current in the model. The sign indicates the direction of that current. View
  // components can use this value to determine how they should behave -- eg, how far to move a voltmeter needle,
  // how bright to make a light bulb, and how fast to move electrons.
  public readonly currentAmplitudeProperty: TReadOnlyProperty<number>;

[pixelzoom]

@arouinfar @samreid How about currentSignalProperty?

[samreid]

That is an improvement, but I wonder what you think of a more general term like currentAmountProperty or equivalent?

[pixelzoom]

We'd also like to remove the word "current", to avoid misconceptions like #118. There is no computation of current in this model, and no modeling of resistance.

@arouinfar and I like normalizedVoltageProperty, which has a range [-1,1]. EMF is in volts, and the computation is emf / maxEMF.

[pixelzoom]

While we have no current or resistance in the model, we unfortuately have things that are named CurrentIndicator CurrentSource to correspond to how they are presented in the UI.

[samreid]

When renaming to normalizedVoltageProperty, consider renaming CURRENT_AMPLITUDE_RANGE to match.

[pixelzoom]

When renaming to normalizedVoltageProperty, consider renaming CURRENT_AMPLITUDE_RANGE to match.

Yes - anything named "current amplitude" will be renamed accordingly.

[pixelzoom]

I'm starting to like normalizedCurrentProperty more than normalizedVoltageProperty.

[pixelzoom]

@arouinfar ready for your review.

I changed to normalizedCurrentProperty throughout the code, and in model.md.

pickupCoil.coil.normalizedCurrentProperty and electromagnet.coil.normalizedCurrentProperty are the 2 places where this name appears in the PhET-iO API. They share the same PhET-iO documentation, shown below. This previously said only "For internal use only", but I think it deserves clarification.

[arouinfar]

Looks good, thanks @pixelzoom.