PhetioGroup tandem name indexing

[pixelzoom]

This came up in a discussion with @arouinfar, in the context of https://github.com/phetsims/fourier-making-waves/issues/6. To be clear, we may not pursue this approach for Fourier. But the problem seems generally important to consider for PhetioGroup, and we'll use Fourier as the example. You may want to run the Java version to familiarize yourself with the Discrete screen.

The Discrete screen in Fourier lets the student specify how many harmonics their waveform has, and what the amplitudes are for those harmonics. N is the number of harmonics, N's range is [1,11], and amplitudes are labeled A₁,...,A_N.

One way we might implement this is a Waveform object that has a dynamic collection of number of NumberProperty instances, one for the amplitude of each harmonic. These Properties would be managed using a PhetioGroup, and would dynamically be created/disposed as N changes.

There are 2 aspects of how PhetioGroup indexes tandem names that seem problematic:

(1) Tandem name indices start from 0. In our example, indexing of the amplitude Properties must start from 1, i.e. amplitude1Property, amplitude2Property,...

(2) Tandem name indices increment by 1. In our example, tandem names must be limited to a specific set: amplitude1Property, amplitude2Property,... amplitude11Property, to correspond to A₁, A₂ ...,A_N.

Questions for the PhET-iO team:

Q1: Are the indexing requirements described about supported by PhET-iO, or something that would need to be added?

Q2: If "need to be added", how about allowing the client to optionally specify the tandem name for createElement?

[samreid]

Q1: Are the indexing requirements described about supported by PhET-iO, or something that would need to be added?

PhetioGroup doesn't support that.

Q2: If "need to be added", how about allowing the client to optionally specify the tandem name for createElement?

For Fourier, I recommend creating amplitude names via string concatenation: `amplitude${n}Property` and creating them statically, not using PhetioGroup.

From the instrumentation guide:

Here is an ordered list of how to approach instrumenting an element that is dynamically created:

1. Does it even need instrumentation? Often instances don't need to be instrumented, or can perhaps be instrumented as a component of their parent (instead of being instrumented themselves)
2. Can it be created eagerly? Allocating dynamic elements on startup simplifies the instrumentation process, especially when supporting PhET-iO state and API validation.
3. Instrument the object dynamically, using PhetioGroup or PhetioCapsule. Use PhetioCapsule for single dynamic instances. Use PhetioGroup for multiple instances of the same type. If you have a use case that is not addressed by one or both of those, then please consult with the PhET-iO subteam, and potentially create a new IO Type suitable for your simulation.

It seems like the amplitude properties fall under level 2.

I think we should avoid trying to create custom names for members of a PhetioGroup, and treat it more like a pool.

[pixelzoom]

@samreid said:

For Fourier, I recommend creating amplitude names via string concatenation: amplitude${n}Property and creating them statically, not using PhetioGroup.

First, this was a general question, and (in talking with @arouinfar) seemed to be a general needed -- to dynamically create elements using the same tandem names, and with indexing that does not start from zero. Fourier was used as a concrete example, and the approach we decide to use there doesn't eliminate the need to discuss this generally. That said...

How to create amplitude Properties (and other elements related to the number of harmonics) is the topic of https://github.com/phetsims/fourier-making-waves/issues/6. What you've described is referred to as "approach (1)", and it's the approach that I've recommended, and I'll make a note that you've recommended this as well. But the decision is up to the PhET designers, and will be discussed in our 11/12/2020 design meeting.

. I think we should avoid trying to create custom names for members of a PhetioGroup, and treat it more like a pool.

If designers can be convinced that there's no need for what was generally described by this issue, then great. If not, then PhetioGroup (or something else) needs to be more than a pool.

Assigning back to the iO team to discuss with designers. I'll be happy to chime in, but whether these features are needed is not my call.

[zepumph]

This was discussed during PhET-iO meeting today.

MK: what about moving this logic out of the tandem names, and into the data of the Property itself.

KP: this is much worse for instructional designers in studio

CM: This would mean subclassing Property to add this data.

CM: what about an option to supply a tandemName in createIndexedMember

SR: if we implement this, then we should be able to give the full tandem name, not just number.

We think that we can achieve this by adding an option to PhetioGroup.createNextElement().

CM: mentioned that another way to implement was with a base class that doesn't have groupMemberIndex, and then the current PhetioGroup would extend that and implement the default numbered tandem names.

@samreid asked if we need to do this now. We feel like it will be best to do this once there is a use case for it. Marking deferred until then.

[pixelzoom]

I would implement this using subclassing, something like this:

/**
 * Creates dynamic elements where the client is responsible for the tandem names.
 */
class PhetioGroup extends PhetioDynamicElementContainer {
  ...
  createNextElement( tandemName, ...argsForCreateFunction ) {...}
}

/**
  * Creates dynamic elements where where the tandem names are automatically creating
  * using an index that is incremented for each new element created.
  */
class IndexedPhetioGroup extends PhetioGroup {

   constructor( createElement, defaultArguments, options ) {

    options = merge( {
      startingIndex: 0,
      ...
    }, options );

     ...
     super( createElement, defaultArguments, options );

     this.groupElementIndex = options.startingIndex;
     ...
   }

    createNextElement( ...argsForCreateFunction ) {
       const tandemName = ... // created based on this.groupElementIndex
       super.createNextElement(  tandemName, ...argsForCreateFunction );
    }
}

[samreid]

In https://github.com/phetsims/center-and-variability/issues/203#issuecomment-1563359653 we discussed tandem indexing and naming.

In https://github.com/phetsims/center-and-variability/issues/203#issuecomment-1564723920 @pixelzoom said:

Perhaps PhetioGroup should have an option to set the starting index, something like groupElementStartIndex: 0. It's currently hard-coded to zero in 2 places:

    // (only for PhetioGroupIO) - for generating indices from a pool
    this.groupElementIndex = 0;
...
    if ( options.resetIndex ) {
      this.groupElementIndex = 0;
    }

@zepumph replied:

That sounds totally fine with me. Should we default it to 1?

OR should we just not have an option and change the start index to 1?

@pixelzoom said:

Would either of those changes require a migration rule?

Anybody want to un-defer this issue?

[zepumph]

Added above, but the default is still 0. That feels like the hard part of this issue.

If we want to change that, perhaps we should also add to the phet-io documentation about this new option. Do we have consensus that this should start at 1?

[pixelzoom]

Difficult to reach consensus without an answer to:

Would either of those changes require a migration rule?

[samreid]

I hardcoded the groupElementStartingIndex to be 1 and ran grunt generate-phet-io-api --stable. The only changes I saw were:

Subject: [PATCH] Move soccer player images to separate files, see https://github.com/phetsims/center-and-variability/issues/222
---
Index: repos/natural-selection/natural-selection-phet-io-api.json
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/repos/natural-selection/natural-selection-phet-io-api.json b/repos/natural-selection/natural-selection-phet-io-api.json
--- a/repos/natural-selection/natural-selection-phet-io-api.json    (revision db30646cde1ed70c3822692e0f4eed47646282ad)
+++ b/repos/natural-selection/natural-selection-phet-io-api.json    (date 1686694032497)
@@ -4207,7 +4207,7 @@
                   }
                 }
               },
-              "bunny_0": {
+              "bunny_1": {
                 "_data": {
                   "initialState": {
                     "_cumulativeHopTime": 0,
@@ -12974,7 +12974,7 @@
                   }
                 }
               },
-              "bunny_0": {
+              "bunny_1": {
                 "_data": {
                   "initialState": {
                     "_cumulativeHopTime": 0,

Since Natural Selection requires migration rules, we would presumably want to migrate that index. But it would probably be best to just leave natural selection alone and let its indexing remain starting at 0.

[pixelzoom]

Since Natural Selection requires migration rules, we would presumably want to migrate that index. But it would probably be best to just leave natural selection alone and let its indexing remain starting at 0.

I agree for Natural Selection.

And in general, for each published PhET-iO sim that uses PhetioGroup, we'll need to decide whether to explicitly set groupElementStartingIndex: 0, or change to 1-based indexing and add a migration rule. The former sounds easier to me, unless there's a great advantage to 1-based indexing.

Here's the list of sims that use PhetioGroup. Search for new PhetioGroup and extends PhetioGroup.

• [ ] balancing-act
• [ ] beers-law-lab
• [ ] charges-and-fields
• [ ] cck-common
• [ ] energy-forms-and-changes
• [ ] energy-skate-park
• [ ] greenhouse-effect
• [ ] john-travoltage
• [ ] natural-selection
• [ ] projectile-motion

[samreid]

Maybe we can leave existing sims listed above starting at 0, and for new sims, we can decide on a case-by-case basis if it should start at 1 or 0. Leaving the default as 0. Can this issue be closed?

[pixelzoom]

I agree that it's best to leave existing sims as 0-base indexing. There are 2 ways to do that:

(1) Leave the default as groupElementStartingIndex: 0 and specify groupElementStartingIndex: 1 as needed going forward. (2) Change the the default to groupElementStartingIndex: 1 and override with groupElementStartingIndex: 0 for the above sims.

Which would you prefer? I'm ambivalent, and would be happy with either default going forward.

[samreid]

Option (3) Make groupElementStartingIndex a required option.

No strong preference here. Let's hear from @zepumph next.

[zepumph]

Wasn't the decision that we like 1-indexing generally for phet-io? If that is the case then (2) is the way to go.

UPDATE: Leaving me assigned to implement.

[pixelzoom]

Like @zepumph, I also thought that we had cconcluded that 1-based indexing was a better default. I was just pointing out that we shouldn't change that in existing sims unless there's a good reason to create more work for ourselves.

[samreid]

What we wrote in the PhET-iO meeting was: https://github.com/phetsims/phet-io/commit/6c476084b3aa4e04e02f26a80587f2352c15a976

Namely:

1. • If the numbering appears in the UI, then the tandem naming should match that numbering.
2. • PhetioGroup uses 0-based indexing, and that is acceptable for all its cases thus far.

Perhaps there was a subsequent meeting I'm not recalling. I don't feel strongly at all, please continue.

[pixelzoom]

I guess I misunderstood. I thought that @samreid was advocating for 1-based indexing as the default. I don't care either way. If 0-based is OK and is the most expedient path to closing this issue, that's fine with me.

[zepumph]

Over to @samreid, if it were on me I would still do (2)

[samreid]

OK I went with (2), @zepumph can you please spot check?

[zepumph]

In my opinion, a starting index of 1 is the way of the future, and so only the spots that are prohibitively annoying to change now (because of migration support) need to stay as 0. With that in mind, shouldn't we revert the following?

https://github.com/phetsims/tandem/commit/38bd5efe92599f98c668c7265f93b3cc63ee90db https://github.com/phetsims/charges-and-fields/commit/04ea11086f5856fa6ead3e9282d1190a0f4168b2 https://github.com/phetsims/balancing-act/commit/8e244399886584802c6ff534c10d035a7fc432dc https://github.com/phetsims/projectile-motion/commit/b8902b2b2e5b8b5284d28a7f1bfddf6f4517333d https://github.com/phetsims/energy-forms-and-changes/commit/599b7246aed3afd1d3673a7d57a80041d4e2fb9a https://github.com/phetsims/john-travoltage/commit/3e45e920455985a1c133ff0af071f2e0a51f6952 https://github.com/phetsims/energy-skate-park/commit/d1a5a7d32e1212625d43ef87a27f17f9b11ee64a https://github.com/phetsims/phet-io-test-sim/commit/177452856c7865ca6154832f689bd98afed239f8 https://github.com/phetsims/greenhouse-effect/commit/c916676f66812f0279ccd8e418399cf9a5f7d755

Then we are keeping only BLL, natural selection, and CCK in the old, 0-indexed paradigm. Thoughts?

[pixelzoom]

Thoughts?

I think that's a question for the designers and developers of those sims. If they migration rules are needed, the decision is easier. For BLL and Natural Selection, I would like to keep them 0-based to avoid migration issues, and because index numbering is a non-issue in those sims.

[arouinfar]

I have not read through everything in this issue, but want to respond to @pixelzoom's https://github.com/phetsims/tandem/issues/226#issuecomment-1599070986 above.

From a design perspective, 1-indexing is only important when the index is prominent in the sim. For example, Fourier: Making Waves labels the amplitude controls A₁, A₂, etc. It's important to keep the index in sync with the view. However, I see no reason why CCK or Natural Selection needs to have 1-indexing. In those sims, I have no idea that I'm looking at battery 7 or bunny 229, so the index is totally irrelevant (again, from a design perspective).

[zepumph]

The above sentiment by @arouinfar makes it sound like there is a general preference towards 0-based indexing, and that 1-based indexing should only be used if necessary. We have decided the opposite, and to go all in on 1-based unless there is a reason to not. This is the most consistent, and best for the cases we have discussed. For example, I have not heard any design preference towards 0-based indexing in any case (for design reasons). That is why I feel like we should think of 0-based group indexing as vestigial, and only keep it in cases where we need to support migration.

[pixelzoom]

That's not what I'm hearing from @arouinfar. What I'm hearing is that in some sims (like CCK and Natural Selection) the indexing doesn't matter, so we really don't care if it's 0-based or 1-based. But in sims where the indexing is "prominent in the sim" (e.g. visible in the UI) then we care, and we typically want 1-based.

So I think we're in agreement that 1-based is the best default going forward. And for sims where indexing doesn't matter (like BLL, CCK, Natural Selection) and is costly to change, we should leave it alone.

[zepumph]

I reverted the commits mentioned in https://github.com/phetsims/tandem/issues/226#issuecomment-1599065034, and as a nice sort of double check, there were no stable API changes from it. I also updated the documentation.

@arouinfar, can you confirm that you feel good about a general shift towards "be 1-indexed always unless there is a reason not to." Feel free to close.

[arouinfar]

So I think we're in agreement that 1-based is the best default going forward. And for sims where indexing doesn't matter (like BLL, CCK, Natural Selection) and is costly to change, we should leave it alone.

I agree 100%. Sorry, I was a bit unclear earlier.

@arouinfar, can you confirm that you feel good about a general shift towards "be 1-indexed always unless there is a reason not to." Feel free to close.

Yes, this sounds good moving forward. Closing.