phetsims / bamboo

Charting library built with Scenery
http://scenerystack.org/
MIT License
3 stars 0 forks source link

ChartCanvasLinePlot should support "holes" in the data #17

Closed samreid closed 3 years ago

samreid commented 3 years ago

LinePlot supports missing data like so:

      // NaN or Infinite components "pen up" and stop drawing
      if ( this.dataSet[ i ].isFinite() ) {

It seems ChartCanvasLinePlot should support it too, so it is easy to switch between plot types.

Discovered in #16

pixelzoom commented 3 years ago

This feature also needs to be documented in LinePlot. I didn't know this feature existed, and would only have discovered it by reading the implementation of updateLinePlot.

samreid commented 3 years ago

I updated CanvasLinePlot so it skips non-finite data the same way LinePlot does, and I added a note of documentation to each. @pixelzoom can you please review?

UPDATE: I should also mention I added some non-finite data to the test harness ?component=ChartCanvasNode

pixelzoom commented 3 years ago

This still didn't seem clear to me, so I took a stab at more clarification and examples in the above commit.

In doing this, I also discovered another issue. A dataSet is consistently described as a {Vector2[]}:

 * LinePlot renders a dataset of {Vector2[]} by connecting the points with line segments.
...
@param {Vector2[]} dataSet

But these non-finite values that can be inserted into a data set are NOT Vector2. So should the type expression throughout be changed to {Array.<Vector2|number>}, with the constraint the a {number} must be non-finite? This also makes me think that using non-finite numbers is the wrong approach. Why not use null? It would be much easier to describe, the type expression would be {Array.<Vector2|null>}, and an example would look like:

[ (0,0), (0,1), null, (0,2), (0,3) ]

instead of:

[ (0,0), (0,1), NaN, (0,2), (0,3) ]

samreid commented 3 years ago

The current implementation only supports Vector2[] and uses Vector2.isFinite() to determine if a value will be rendered or not. This is a natural fit for time series data, where one dimension is known and one may be undefined, such as new Vector2(7.5, NaN). This strategy is used in both Wave Interference and Circuit Construction Kit. For bamboo it doesn't seem important to know that a measurement was NaN at a certain time, maybe other situations would leverage that (or maybe not).

On the other hand, an array of finite Vector2 | null also seems reasonable. Would we want to collapse multiple null values? For the purposes of this chart, [ (0,0), (0,1), null, (0,2), (0,3) ] would render the same as [ (0,0), (0,1), null, null, (0,2), (0,3) ].

pixelzoom commented 3 years ago

Ah, I see -- this.dataSet[ i ].isFinite() still involves a Vector2. Thanks for clarifying. I've fixed the doc in the above commit.

It seems odd (and as I've unintentionally demonstrated, confusing) to have to create a non-finite Vector2 instance -- and either know or investigate how Vector2 isFinite is implemented. Using null seems clearer, easier to explain, and creates fewer objects. If we switched to using null to represent a gap, I see no reason to collapse consecutive null values, for the same reason that there's no reason to collapse consecutive non-finite Vector2 instances.

EDIT: If the "non-finite Vector" strategy is important to WI/CCK, then feel free to leave as is. But it seems like it would be (almost as) easy to just insert null if one dimension is unknown.

Also... The doc for Vector2 isFinite is a tad confusing:

* Whether all of the components are numbers (not NaN) that are not infinity or -infinity.

Better would be:

 * Returns false if either component is NaN, infinity, or -infinity. Otherwise returns true.
samreid commented 3 years ago

I revised the documentation as recommended, and converted to use null instead of non-finite Vector2 for the data. I found that CCK was using these renderers from bamboo, but Wave Interference is not, so Wave Interference didn't need to be updated. I was concerned there would be a problem in CCK because it used the "time" value of a non-finite Vector2 to know when to remove a data point from the data set, but I tested removing null points and didn't see any problems, and it seems like it will be OK.

Ready for review.

pixelzoom commented 3 years ago

Looks good, thanks for handling. Closing.