search

phetsims / graphing-quadratics

"Graphing Quadratics" is an educational simulation in HTML5, by PhET Interactive Simulations.
MIT License
1 stars 4 forks source link

apply phetioFeatured #112

Closed samreid closed 5 years ago

samreid commented 5 years ago

In https://github.com/phetsims/graphing-quadratics/issues/14#issuecomment-424045119 we identified the PhET-iO features we wanted to support for this sim. In https://github.com/phetsims/phet-io-wrappers/issues/143 we introduced a new PhetioObject option to indicate featured instances. We should identify them as such:

This is not essential for 1.0. Testing this requires the studioc branch of phet-io-wrappers. Tagging @pixelzoom so he is aware I'm looking into this.

pixelzoom commented 5 years ago

Is this a prerequisite for 1.0 release?

pixelzoom commented 5 years ago

@samreid said:

In phetsims/phet-io-wrappers#143 we introduced a new PhetioObject option to indicate featured instances.

What are " featured instances"? I see no description of the term here or in phetsims/phet-io-wrappers#143.

In PhetioObject, I see:

phetioFeatured: false // True if this is an important instance to be "featured" in the PhET-iO API

That helps a little, but how and where is an instance "featured" in the API?

pixelzoom commented 5 years ago

Looking through the above commits, https://github.com/phetsims/sun/commit/44e0a692e891d3e64bb10576de014f754b028bb4 is a bit of a red flag. It adds Slider option enabledPropertyOptions, which is ignored if the client sets option enabledProperty. We have lots of places in common code where a component instantiates a default Property if the client does not provide one. Will we need to use this pattern in all of those cases?

Side note... Slider's default for options.enabledProperty should be changed from Property to BooleanProperty.

samreid commented 5 years ago

Is this a prerequisite for 1.0 release?

This was addressed in the initial comment where it says "This is not essential for 1.0."

That helps a little, but how and where is it featured in the API?

The new version of studio (in the branch referred to in https://github.com/phetsims/phet-io-wrappers/issues/143 has an option to only show "Featured" items.

Looking through the above commits, phetsims/sun@44e0a69 is a bit of a red flag.

We should add checks that enabledPropertyOptions and enabledProperty options are mutually exclusive.

Will we need to use this pattern in all of those cases?

Perhaps we would need to use this pattern for instrumented subcomponents that need to be customizable, which can be created or passed in.

Side note... Slider's default for options.enabledProperty should be changed from Property to BooleanProperty.

Sounds good to me!

pixelzoom commented 5 years ago

I've made related Slider improvements in https://github.com/phetsims/sun/issues/418.

pixelzoom commented 5 years ago

After today's PhET-iO dev call with @kathy-phet, the decision was made to rollout Graphing Quadratics 1.0 with the redesigned Studio that has the "Featured" feature. So this issue needs to be completed. Priority raised to high.

samreid commented 5 years ago

instrument drag handlers for manipulators

What are the "manipulators"?

UPDATE: I discovered VertexManipulator, FocusManipulator and PointOnParabolaManipulator. I passed phetioFeatured: true to their input listeners, but it is not cascading to the nested emitters. I may need to resort to phetioComponentOptions again...

pixelzoom commented 5 years ago

... I may need to resort to phetioComponentOptions again...

Gross.

samreid commented 5 years ago

Oh wait, there is a more palatable established pattern for this case... Hold for commits.

samreid commented 5 years ago

instrument slider drag handlers

This is instrumented but not "featured". But the associated model properties are already featured. It is really important to feature the slider input listener? If we do make the slider thumb drag handler featured, do we also want to make other slider input listeners featured, such as clicking in the track?

kathy-phet commented 5 years ago

@samreid and @pixelzoom - I'm feeling like this list is what we wanted to instrument for this sim, but not necessarily what we might want to feature? Feature seems like it would be the most basic set of things we would imagine someone wanting to do. Some of these were for "advanced use" cases. I'm starting to feel like a PhET-iO design includes both the full brainstorm and then the "what should be featured" list out of that list.

pixelzoom commented 5 years ago

@kathy-phet said:

... I'm starting to feel like a PhET-iO design includes both the full brainstorm and then the "what should be featured" list out of that list.

Agreed. But the concept of "featured" didn't exist when the PhET-iO design was done. Suggestions on how to proceed? Can what is "featured" be reviewed/revised as part of the general PhET-iO review to be done in https://github.com/phetsims/graphing-quadratics/issues/115?

kathy-phet commented 5 years ago

Is there space in Design Meeting this week to refine the list for "featured"?

pixelzoom commented 5 years ago

The first thing would be to make sure that @amanda-phet is aware that the "Featured" feature exists, so she has some time to think about it. As far as I know, she has been out of the loop.

amanda-phet commented 5 years ago

Thanks for looping me in, @pixelzoom ! I'm happy to have a design meeting about this. Is this the first sim that has a featured list? If so, we might want to invite Amy as well so we can both learn more about PhET-iO design (since we are responsible for contributing to these kinds of decisions).

kathy-phet commented 5 years ago

Yes, first sim with a featured list. Kathy

Sent from my iPhone

On Nov 27, 2018, at 8:14 PM, Amanda McGarry notifications@github.com<mailto:notifications@github.com> wrote:

Thanks for looping me in, @pixelzoomhttps://github.com/pixelzoom ! I'm happy to have a design meeting about this. Is this the first sim that has a featured list? If so, we might want to invite Amy as well so we can both learn more about PhET-iO design (since we are responsible for contributing to these kinds of decisions).

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/phetsims/graphing-quadratics/issues/112#issuecomment-442303785, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AE3FZDPWQmSqrxYB7fIcu4BF0caDKZR6ks5uzf90gaJpZM4YngeI.

pixelzoom commented 5 years ago

PhET-iO version has been deferred and will not be published with 1.0, see https://github.com/phetsims/graphing-quadratics/issues/101#issuecomment-442939061. So this issue can be deferred.

pixelzoom commented 5 years ago

Reviewed/discussed a bit at 11/29/18 design meeting. But this should be revisited by @amanda-phet when it's time to publish the first PhET-iO version.

kathy-phet commented 5 years ago

I've moved this to a google doc, and am working on it here: https://docs.google.com/document/d/1OfHhIDuaBt-hhXVtpnJgM3Wm0iByJ7dmXd-2UkphUR4/edit

zepumph commented 5 years ago

Removing the deferred label now that phet brand was published. We should finish this up.

pixelzoom commented 5 years ago

Happy to finish this up. But (afaik) a PhET-iO version of Graphing Quadratics is not on the release schedule. And @amanda-phet and @kathy-phet need to specify what should (and should not) be "featured" before we make any code changes.

pixelzoom commented 5 years ago

12/13/18 phetio meeting:

pixelzoom commented 5 years ago

Here's a password-protected PhET-iO dev version with the latest version of Studio:

https://phet-dev.colorado.edu/html/graphing-quadratics/1.1.0-dev.2/phet-io/

The direct link to Studio (also password protected) is:

https://phet-dev.colorado.edu/html/graphing-quadratics/1.1.0-dev.2/phet-io/wrappers/studio/

Direct link to Studio has been added to https://docs.google.com/document/d/1OfHhIDuaBt-hhXVtpnJgM3Wm0iByJ7dmXd-2UkphUR4/edit

pixelzoom commented 5 years ago

Email from @amanda-phet:

Kathy and I are finished meeting about the PhET-iO featured list for GQ. This document list everything that we think might want to be included in the featured list, but there are also some questions. It might be good to review it before Thursday's design meeting.

https://docs.google.com/document/d/1OfHhIDuaBt-hhXVtpnJgM3Wm0iByJ7dmXd-2UkphUR4/edit#

Assuming that things highlighted in yellow make up the complete list of things that need to be addressed… I’ve commented on all such things in the document. Most of the requested changes are in common code and will affect all sims.

samreid commented 5 years ago

I'm concerned after seeing the complexity in the GQ PhET-iO featured design document. Manually comparing this to the instrumented simulation will be time consuming and error-prone. I recommend that we investigate a new format for these design documents that is also machine-readable and hence can be automatically checked for coverage and support.

pixelzoom commented 5 years ago

@samreid perhaps you should attend today's design meeting?

pixelzoom commented 5 years ago

We had a design meeting on 2/28/19 and started reviewing the Google doc.

pixelzoom commented 5 years ago

I've done as much of the instrumentation as I can, and have marked up the Google doc accordingly. There are many common-code issues that need to be addressed (listed in https://github.com/phetsims/graphing-quadratics/issues/112#issuecomment-468445013). And there are several Google doc comments that need to be discussed.

@amanda-phet next steps?

pixelzoom commented 5 years ago

I have implemented the majority of the "featured" specification, see PhET-iO Featured List: Graphing Quadratics.

All remaining requirements involve common-code design and implementation tasks, as indicated in the comments attached to the specification. The blocking issues are:

pixelzoom commented 5 years ago

@amanda-phet @kathy-phet I've done as much work as possible on this. So this is ready for review. Verification is a manual process. You'll need to go through the design document, and compare anything with ✅ next to it to what you see in Studio.

The remaining issues require common-code design decisions, or changes to Studio. You'll have to decide whether those issues are prerequisites for publication. Those issues are:

pixelzoom commented 5 years ago

This has been ready to review for 29 days. But before that happens, perhaps we should discuss the implications of the work that's being done on Studio.

When it's possible to specify metadata using Studio, I think that we should essentially start over with this issue. I.e.,

@amanda-phet @kathy-phet @samreid does that sound like a reasonable plan?

samreid commented 5 years ago

Remove the explicit metadata from this sim's code.

Putting the metadata in the code is the most efficient way to factor it out and provide long-term maintainability. Please refer to https://github.com/phetsims/phet-io/issues/1440#issuecomment-474634954 "Strategy C" that indicates that some problems that increase in severity with the size of the overrides file.

Roll back the distasteful changes that I had to make to common code in order to propagate that metadata via options.

Common code still needs a way to specify the baseline metadata, and as per my previous comment, it is sometimes advantageous to factor out metadata into the code. But I would like to understand your point better, can you please refer to a distasteful change in common code so I can see it in context?

pixelzoom commented 5 years ago

Putting the metadata in the code is the most efficient way to factor it out and provide long-term maintainability.

I thought the point of adding metadata editing to Studio was so that a developer wouldn't have to put the metadata in the code in the first place. If Studio provides a description of the metadata, why would it be duplicated (or worse, different) in the code?

can you please refer to a distasteful change in common code so I can see it in context?

The design specified "featured" for the visibleProperty of the expandCollapseButton for all AccordionBox instances. This required the changes in https://github.com/phetsims/graphing-quadratics/commit/c1f0407a55112e240890c20fe0fb8d5859a8a701 and https://github.com/phetsims/graphing-quadratics/commit/8eb6f3bbfb9fd4759f6a381408a2b5ccce5df613, where I added nested options solely for the purposed of featuring a deeply buried Property. And it required phetioComponentOptions to do so, which imo is generally distasteful/ugly.

samreid commented 5 years ago

I thought the point of adding metadata editing to Studio was so that a developer wouldn't have to put the metadata in the code in the first place.

Right, I agree that will come in very handy in future sim instrumentation. But I see it as a step backwards to roll back the work you already did for this. To clarify, I see a significantly lower cost to specify metadata through the overrides file, but an overall benefit to specifying the metadata in the simulation. Since that up-front cost is already paid for Graphing Quadratics, I'd recommend to keep it. If the design team wants changes from what is already there, that could be done through the overrides (to save time).

If Studio provides a description of the metadata, why would it be duplicated (or worse, different) in the code?

I'm really confused and perhaps in-person conversation tomorrow would help. If I understand correctly, Graphing Quadratics already has the correct "in-simulation" baseline instrumentation and an empty "overrides" file. That means there is no need for duplication or possibility for differing values. Once we introduce the overrides file, this will create metadata values that differ from the baseline.

And it required phetioComponentOptions to do so, which imo is generally distasteful/ugly.

In the initial discussion for "Strategy C" it seemed plausible that Strategy C would eliminate the need for phetioComponentOptions completely, and we were excited about the possibility. However, after getting more experience with this approach, I think we still need to keep it or something like it instead of relying 100% on overrides files. For the sake of discussion, let's say that one day we decide all checkbox "visibleProperty" should be phetioFeatured: true. If this is done solely in the override files, then we would need to manually iterate through every checkbox and every simulation and make this change (N*M sites). If this is done through phetioComponentOptions or an equivalent strategy, this change needs to be made in only one place, then the baselines can be updated by running for-each.sh active-runnables grunt generate-phet-io-elements-baseline.

pixelzoom commented 5 years ago

Since that up-front cost is already paid for Graphing Quadratics, I'd recommend to keep it. If the design team wants changes from what is already there, that could be done through the overrides (to save time).

The up-front cost is not fully "paid for". The implementation has not been verified to be correct, and I'll be surprised if it is correct or isn't changed. And changing it through overrides will result in exactly what I'd like to avoid; metadata in the sim's code that does not correspond to the specification. I don't see this as a way "to save time"; I see it as a maintenance issue in code that I'm responsible for.

If I understand correctly, Graphing Quadratics already has the correct "in-simulation" baseline instrumentation and an empty "overrides" file.

Graphing Quadratics instrumentation has not been verified to be correct. It would have an empty overrides file only if review results in zero changes, which I think is unlikely. And you just said that an acceptable way to fix anything that's incorrect is "If the design team wants changes from what is already there that could be done through the overrides".

In the initial discussion for "Strategy C" it seemed plausible that Strategy C would eliminate the need for phetioComponentOptions completely, and we were excited about the possibility. However, after getting more experience with this approach, I think we still need to keep it or something like it instead of relying 100% on overrides files. ...

I agree that common code will likely have a need for metadata in the code. But setting that metadata can be done directly on the instances, customizing (overriding) that metadata via options propagation won't be necessary, since overrides will be done via Studio.

samreid commented 5 years ago

Thanks for clarifying, I see that it could be problematic to specify one thing in the simulation code then override that in the overrides.

But setting that metadata can be done directly on the instances

Yes, it is simple to specify metadata directly on the instances through the overrides instead of via the phetioComponentOptions, but the override file doesn't offer any possibility for factoring out or abstraction. In the bottom part of https://github.com/phetsims/graphing-quadratics/issues/112#issuecomment-479725995 I provided an example which we haven't discussed:

For the sake of discussion, let's say that one day we decide all checkbox "visibleProperty" should be phetioFeatured: true. If this is done solely in the override files, then we would need to manually iterate through every checkbox and every simulation and make this change (N*M sites). If this is done through phetioComponentOptions or an equivalent strategy, this change needs to be made in only one place, then the baselines can be updated by running for-each.sh active-runnables grunt generate-phet-io-elements-baseline`

To elaborate, let's say we have 18 instrumented simulations, each with an average of 7 checkboxes. That means PhET-iO designers would have to open up studio in admin mode 18 times (once for each sim), then search through and find all checkboxes without missing any of them, then save and commit each of the override files. Perhaps that could be completed in an hour, with a low but nonzero risk that a checkbox was missed. If we use the phetioComponentOptions perhaps it would take 10 minutes to add phetioComponentOptions: { visibleProperty: { phetioFeatured: true } } and run for-each.sh active-runnables grunt generate-phet-io-elements-baseline? Then for maintainability, instead of having phetioFeatured:true copied to 18*7=126 places, it would occur in 1 place. But I think I'm misunderstanding part of your argument, so that's why I wanted to clarify and discuss. Happy to discuss in person tomorrow if that's preferable.

pixelzoom commented 5 years ago

Relevant notes from 4/4/19 phet-io meeting:

Studio metadata editing features are looking really nice. When Studio is ready, I'll make a pass through Graphing Quadratics and adjust which metadata is specified in baseline (code) vs overrides (Studio). Then I'll pass it back to @amanda-phet and @kathy-phet for verification.

My priority next week is Gas Properties, and then I'm on vacation for 2 weeks. So the soonest that I will realistically get to this is early May. Unless I hear otherwise, I'll get to this whenever Studio is ready, or early May, whichever comes last.

It would also be helpful if @kathy-phet or @ariel-phet could provide guidance on when the PhET-iO version of this sim needs to be published.

pixelzoom commented 5 years ago

Notes from 5/2/19 status meeting...

The plan now is to have @kathy-phet and @amanda-phet identify desired instrumentation of common components. Do that work, then tackle Graphing Quadratics.

pixelzoom commented 5 years ago

@amanda-phet and @kathy-phet specified desired common-code instrumentation in Graphing Quadratics: PhET-iO Featured List. My feedback via email to @amanda-phet was:

I looked over the doc and yes, fairly straightforward. We’ll talk more at 10:00 on Wednesday, but here are some general comments/questions, which you might want to run past KP and AP.

(1) “Global” decisions should live in their own PhET-iO design decisions, not a sim-specific doc like Graphing Quadratics. Sim-specific PhET-iO design docs would then reference the global doc. Let’s decide where that doc should live, what it should be named, etc.

(2) We should create GitHub issues for any global work that doesn’t already have a GitHub issue. This will allow us to assign the work, and track the changes. I’ll be happy to create the issues.,

(3) Collectively, this is a big chunk of work. Who is doing the work? Is the assumption that I’m going to do it while preparing Graphing Quadratics? What is the priority?

@amanda-phet and I also discussed on Zoom.

@amanda-phet moved the common-code specification to PhET-iO Design for Common Components

Next step is to have a meeting about common-code instrumentation. Assigning this issue to @amanda-phet and @kathy-phet to make that meeting happen.

pixelzoom commented 5 years ago

6/6/19 dev meeting:

This issue is now part of the GQIO-oneone milestone. I'll be revisiting GQ, moving some instrumentation out of code, and instrumenting via Studio. Then passing it on to @kathy-phet and @amanda-phet for review.

@samreid will assign this issue to me when PHET-iO/Studio are ready for me to proceed.

samreid commented 5 years ago

@zepumph said:

Michael Kauzmann [12:05 PM] I see review comments in https://github.com/phetsims/phet-io/issues/1466#issuecomment-500119005, but those shouldn't block your work in GQ It has been in my court, but I have been off phet-io since it was assigned to me. I would say you are good to go in GQ. Thanks for checking in.

pixelzoom commented 5 years ago

I did grunt dev --brands=phet-io to publish 1.1.0-dev.5 before starting this work, so I have a baseline of where things were at.

I asked the PhET-iO team for guidance on the general process of iterating on GQ.

@samreid provided a link to https://docs.google.com/document/d/116HkWEDCmw59PLZETiEUHlxAQf4aUpDHCZIHulIbPmQ/edit.

@zepumph provided this info:

Options to consider moving from GQ code to Studio, with number of occurrences:

Here’s the iterative process that I arrived at for moving metadata from code to Studio:

  1. remove metadata in code
  2. run “grunt update”
  3. reload Studio
  4. navigate to phetioID in Studio tree
  5. add metadata
  6. copy-paste console to overrides.js
  7. reload Studio
  8. navigate to phetioID in Studio tree to verify
  9. repeat
pixelzoom commented 5 years ago

Summary of specific work:

Generalization:

The iterative process described in https://github.com/phetsims/graphing-quadratics/issues/112#issuecomment-500983067:

pixelzoom commented 5 years ago

Completion of this work is blocked by https://github.com/phetsims/tandem/issues/89 (phetioReadOnly bug).

samreid commented 5 years ago

Thanks for your notes and analysis. https://github.com/phetsims/tandem/issues/89 has a proposed fix and is ready for review.

kathy-phet commented 5 years ago

Sounds like this is going pretty well! Thanks all!

pixelzoom commented 5 years ago

https://github.com/phetsims/tandem/issues/89 was fixed, so I continuing with the remaining work for phetioReadOnly. See the above commit.

Looking at the PhET-iO design doc and the uses of phetioReadOnly:true in the code... The goal was to prevent access to a Node's visibleProperty, since it's intended to be controlled via a checkbox and a view-specific Property. For example, if you want to show/hide the vertex point o the graph, you should be setting viewProperties.vertexVisibleProperty which is used by the "Vertex" checkbox, not the Node's visibleProperty. So I moved the phetioReadOnly: true from the Node to its visibleProperty, and didn't have to deal with the other PhetioObject Properties (enabledProperty, pickableProperty,...)

pixelzoom commented 5 years ago

I've completed my work on GQ, and published 1.1.0-dev.6 as a record of the work.

The next step is for @amanda-phet and @kathy-phet to review and make corrections/changes, using Studio. When you commit to the graphing-quadratics repo, please reference this issue number in your commit messages. That will attach your changes to this issue, which will make it easier for me to review.

pixelzoom commented 5 years ago

@amanda-phet and I walked through (on Zoom) what needs to be done next.

amanda-phet commented 5 years ago

I reviewed and brought up questions directly with @pixelzoom and also shared generally with the team in the slack channel. I will make a few corrections as well as changes using Studio later today, except for buttons (since that was was related to a change in sun that needs to be fixed, see 1 below).

Here is what I shared in the slack channel:

  1. I believe the common code spec we discussed (for featuring elements by default) did not include enabledProperty for buttons, it was only supposed to feature the visibleProperty. I realized this when going through GQ because it seemed unnecessary for every button I came across. MK fixed this
  2. We discussed featuring simName.navigationBar.phetButton.pickableProperty but in GQ it was actually simName.navigationBar.phetButton.enabledProperty. Was this intentional? Makes sense to me, but just doesn’t match the common components spec. discuss 6/13/19 with the group
  3. I found 1 or 2 errors from what CM did, plus a bunch of default featured items I need to un-feature. I will make the 2 corrections today and wait on the other elements until after a discussion (see 5 below). corrections were committed in the overrides file
  4. I think there is a bug with the accordion box expand/collapse button but @pixelzoom said he was making an issue about it. Currently, changing the visibility of that button does exactly what we requested (you can’t close the accordion box). Since it’s a button (?) the enabledProperty was also featured, and when I checked that box the title bar was still clickable. I’m going to remove that enabledProperty from the featured set, but it still should probably be addressed. See https://github.com/phetsims/sun/issues/514.
  5. I think we need another larger design meeting to discuss this process and the decisions about common code elements. will try to meet 6/13/19 even though KP is gone so that we can continue to make progress