Suppressing warning messages

[MatthijsBon]

Issue type
Core Library & Cose Layout.

Bug report

Environment info

• Cytoscape.js: v3.3.1:
• Browser: Chrome 71.0.3578.98 (Official Build) (64-bit):

Current (buggy) behaviour

I think this has to do with both the 'cose' layout as well as the core library. The main concern is adding nodes dynamically (using elements.forEach(function (element) { cy.add(element) })) or adding directly to the graph (using { (..), "elements": elements, (..) }). When adding directly to the graph, the core library has no problems with adding the nodes using the 'cose' layout, but when adding dynamically, a warning is shown in the console that nodes probably overlap and therefore edges are impossible to draw.

I can imagine edges are impossible to draw when at first they overlap, but in the end, the layout determines the position of the nodes and then they don't overlap anymore. Even when using something like cy.batch(function () {}) to add all nodes (dynamically) at once and then run the layout, the warnings are still shown.

This does not happen for 'breadthfirst' or 'random' layouts and (as far as I have tested) only for 'bezier' and 'straight' edge styles.

Desired behaviour

Some way to suppress these warnings when adding nodes dynamically, while the layout is responsible for the position of nodes.

NOTE: I did find a workaround: a (random) position when adding nodes solves the warning.

Minimum steps to reproduce

https://jsbin.com/manukazata/1/edit?js,console,output

[maxkfranz]

What is the motivation for suppressing the warnings? Is there a performance impact?

[MatthijsBon]

Well not exactly performance impact, but showing all these warnings in the console is a pain in the ***.

[maxkfranz]

That makes sense, but disabling the warnings comes at a cost: When a dev creates an edge and its not visible because of the positions of the connected nodes, it would be confusing to not have the warning.

Maybe the warnings could be collated somehow.

[maxkfranz]

I guess the warning could be suppressed automatically if the connecting nodes both have the default (0, 0) position.

Collation could more generally be done within a time threshold, but I'd be wary of edgecases.

[MatthijsBon]

Yes, that would be a great solution I think! I'm not that well into the codebase of Cytoscape to submit a Pull Request, but I could try with some pointers maybe?

[maxkfranz]

There is some information here: https://github.com/cytoscape/cytoscape.js/blob/unstable/CONTRIBUTING.md

The warning code is here:

cytoscape.js/src/extensions/renderer/base/coord-ele-math/edge-control-points.js

Lines 127 to 138 in d0d70c2

	BRp.checkForInvalidEdgeWarning = function( edge ){
	var rs = edge._private.rscratch;
	if( !is.number(rs.startX) || !is.number(rs.startY) || !is.number(rs.endX) || !is.number(rs.endY) ){
	if( !rs.loggedErr ){
	rs.loggedErr = true;
	util.warn('Edge `' + edge.id() + '` has invalid endpoints and so it is impossible to draw. Adjust your edge style (e.g. control points) accordingly or use an alternative edge type. This is expected behaviour when the source node and the target node overlap.');
	}
	} else {
	rs.loggedErr = false;
	}
	};

[Spaxe]

I'm not sure if checking the "default" (0, 0) position is good enough. Bumping into this same problem:

Edge `696` has invalid endpoints and so it is impossible to draw.  Adjust your edge style (e.g. control points) accordingly or use an alternative edge type.  This is expected behaviour when the source node and the target node overlap.

I can see edge 696 on my screen and it's drawn. I'm also using cose, so I suspect this warning was just a problem during animation.

I did a dump at line 132 in the edge-control-points.js above for rs, and this is what I can see:

{
  "source": { "x": -0.671090240243881, "y": 0.7413756736293753 },
  "target": { "x": -0.9993383998881852, "y": -0.03636980215675652 },
  "allpts": [
    300.4806980317775,
    787.4451734721404,
    305.7636176983382,
    704.2101958135996
  ],
  "haystackPts": [
    300.4806980317775,
    787.4451734721404,
    305.7636176983382,
    704.2101958135996
  ],
  "midX": 303.12215786505783,
  "midY": 745.82768464287,
  "lastCurveStyle": "bezier",
  "edgeType": "straight",
  "haystack": true,
  "srcArrowAngle": 0.06338492479368618,
  "midtgtArrowAngle": -3.078207728796107,
  "midDispX": 5.282919666560701,
  "midDispY": -83.23497765854074,
  "midsrcArrowAngle": 0.06338492479368618,
  "tgtArrowAngle": -3.078207728796107,
  "targetLabelAngle": 0,
  "sourceLabelAngle": 0,
  "labelAngle": 0,
  "badBezier": false,
  "lastSrcCtlPtX": 0,
  "lastSrcCtlPtY": 0,
  "lastSrcCtlPtW": 32,
  "lastSrcCtlPtH": 32,
  "lastTgtCtlPtX": 0,
  "lastTgtCtlPtY": 0,
  "lastTgtCtlPtW": 32,
  "lastTgtCtlPtH": 32,
  "lastEdgeIndex": 0,
  "lastNumEdges": 1,
  "lastCtrlptDists": null,
  "lastCtrlptWs": "0.5",
  "lastSegmentDs": "20px",
  "lastSegmentWs": "0.5",
  "lastStepSize": 16,
  "lastLoopDir": -0.7853981633974483,
  "lastLoopSwp": -1.5707963267948966,
  "lastEdgeDistances": "intersection",
  "lastSrcDistFNode": 0,
  "lastTgtDistFNode": 0,
  "lastSrcEndpt": "outside-to-node",
  "lastTgtEndpt": "outside-to-node",
  "lastSrcArr": "none",
  "lastTgtArr": "triangle-backcurve",
  "lastLineW": 1,
  "lastArrScl": 0.75,
  "srcIntn": [],
  "tgtIntn": [],
  "srcManEndpt": {
    "name": "source-endpoint",
    "value": "outside-to-node",
    "strValue": "outside-to-node"
  },
  "tgtManEndpt": {
    "name": "target-endpoint",
    "value": "outside-to-node",
    "strValue": "outside-to-node"
  },
  "endX": null,
  "endY": null,
  "arrowEndX": null,
  "arrowEndY": null,
  "startX": null,
  "startY": null,
  "arrowStartX": null,
  "arrowStartY": null,
  "badLine": true,
  "loggedErr": true
}

How would one surpress this warning? Maybe during animation, don't warn, but if animation has finished and it's still not visible?

[Spaxe]

Yep, checking for animation suppresses the warning properly, at least for this use case. PR coming.

[maxkfranz]

Whether the edge is animated still isn't enough information to suppress the warning properly.

The warning is there because there are several cases for edges, with different property value combinations, that make the edge impossible to draw. It can be confusing to a consumer without the warning: Did I as the consumer do something wrong or is the library broken?

So then:

(1) The warning must still be shown for all cases except those that are obviously not caused by user error.

(2) The warning suppression logic must not impact performance significantly.

By suppressing the warning for animated elements, (1) is violated. The consumer can animate elements by ele.animation() -- it is not always a layout causing that. Generally, the more sophisticated the suppression rules become, the more (2) is violated.

[Spaxe]

Thanks @maxkfranz. To satisfy both (1) and (2), perhaps the warning can be printed just once - for the first edge that was not drawn, so as not to fill the console with similar information, thus becomes noise?

[maxkfranz]

For layout that makes sense, for other cases it could be confusing to see a missing edge but have no warning for it.

I think the simplest option is to have some init option to suppress all warnings warnings: bool -- basically making util.warn() a nop. If you opt out, you lose out on useful warning messages. But if you don't like warnings generally, then you probably want to opt out of all of them anyway.

[Spaxe]

Warnings are useful. I wouldn't opt out completely - someone may add it, commit the code, and forget about it.

Instead I suggest that the warnings be printed once per cytoscape instance, instead of per edge/node. That still satisfies 1 & 2, and it doesn't have the need to be configured.

[maxkfranz]

If it's per-cy rather than per-ele, then (1) is no longer generally true: For example, you could have one edge that is obviously wrong cause the warning, and then later another non-obvious edge doesn't get any warning.

Warnings are useful. I wouldn't opt out completely - someone may add it, commit the code, and forget about it.

To me, that's an argument against hard-coding your options --- rather than an argument against the existence of an option itself. If you're worried about that case, you should use an environment variable at build time, e.g. warnings: process.env.NODE_ENV !== 'production'. That way, developers get warnings for debug builds but there are no warnings in production builds.

[Spaxe]

I agree that is one safeguard option against hard-coded options. What I am advocating is making it impossible to make that set-and-forget mistake, and not having that configurable option.

If I may offer a compromise: make util.warn debounce. This way, the console will print less warnings (and therefore less noise), and it will collate multiple warnings that came in a short period of time. I can make a PR if you think this is a good compromise.

[maxkfranz]

The only way I could see debouncing working generally on util.warn() without violating (1) is if the warnings are debounced per unique message.

Imagine this sequence, at around the same time: A, A, B, C

I would expect this output: A, B, C

For this particular warning, we already have this behaviour by limiting this warning per element.

Debouncing could work to reduce the overall count of warnings if --- rather than debouncing util.warn() itself --- you debounce each class of warning separately. Each class would need its own collation logic. For invalid endpoints, for example, you'd need to maintain a list of elements per debounce cycle.

[Spaxe]

Yes, that's exactly what I was thinking.

[Spaxe]

Is there a list of warning classes somewhere?

[maxkfranz]

Is there a list of warning classes somewhere?

No, there is just a set of calls to util.warn(). They aren't organised beyond that, because they haven't had to be thus far.

[Spaxe]

I gave this a try for an hour and ran out of time. Just wanted to document my findings so far.

1. Discounting dist/, node_modules/, and test/, there are 25 uses of util.warn(). All of them appear to be slightly different in messaging. A rough guess is that there are 22 classes of warnings in Cytoscape.js

2. Therefore, it is by far the easiest to write a function warnDebounced() that would replace some of the more noisy warning calls. However, lodash.debounce does not have an accumulator, meaning the previous warning context would be lost, and we can't collate information to be printed just once.

3. Logically, the most noisy function is the one in edge control points, so I looked at if I can constraint this PR to just this function. Unfortunately the scratch pad is stored per edge, meaning that they do not know each other's existence. A naive way to write this is to store the edge IDs first, flag them for invalid endpoints, and then at the end loop through all of the flagged edges and print the warning. However, that still means the warning would be printed once per animation frame, even debounced.

4. I ran out of time and decided that this is too hard to tackle with the present constraints and the one hour I gave myself to do this.

That said, I am currently not using animations in our production code, and only displaying the graph after layout animation has finished, and it is not noisy anymore... so I will sit on this one for now.

[maxkfranz]

For (3), a separate collection could be used to store the list of edges. The debouncing could be independent of ticks, but this raises another point: The debounce time can't be specified to be very long, or the utility of the warning is decreased: Knowing when the edge is invalid is useful information, especially if you're dragging it around.

[jri]

To me This is expected behaviour when the source node and the target node overlap makes no sense. When it is expected behavior why issue a warning in the first place?

While moving nodes it happens a lot that "source node and the target node overlap" and is no problem.

In my experience 100% of these warnings are "false alarm" and clutter the dev console. It hinders recognizing the real warnings issued by my app.

I think @maxkfranz already suggested a perfect simple yet flexible solution:

I think the simplest option is to have some init option to suppress all warnings warnings: bool

A big thumb up for this!

[maxkfranz]

To me This is expected behaviour when the source node and the target node overlap makes no sense.

I checked the control point code, and there's a condition to check for node overlap. That state could be stored for the warning: If the nodes overlap, we don't need to print the warning. That case should -- I hope -- be fairly obvious to a developer.

The main purpose of the warning is to let the dev know when he specified style properties that sometimes make the edge impossible to draw: He should update his stylesheet to work more generally with his data.

So if we can automatically eliminate all the node overlap cases to reduce noise, this might be an acceptable compromise.

I still think the option to turn them all off is a good idea, especially for when a dev makes a prod build of his app.

[jri]

Thank you for detailed response!

I still think the option to turn them all off is a good idea, especially for when a dev makes a prod build of his app.

+1

[Spaxe]

I should be able to whip up a PR today to add an option to turn them all off.

[maxkfranz]

The refactoring for the following issue makes it so the invalid edge warning is not shown when the nodes overlap: Clean up control point calculation a bit #2300

[maxkfranz]

The feature/global-warnings-toggle branch has an addition of cytoscape.warnings() -- which is both a setter and a getter.

The benefits of this approach are:

• It's automatically applied to all cy instances that you create.
• It can be changed dynamically.
• It's automatically applied to all util.warn() calls.
  • The rest of the code doesn't have to change.
  • There is no risk of someone accidentally forgetting to check the flag in future code additions.
• It doesn't create the possibility of cyclic dependencies on the core or the passing of extra cy refs.

I initially thought that it would be simpler to use an init option like options.warnings, but that might be more difficult in practice.

[jri]

@maxkfranz Congratulations for 3.5.0!

The new cytoscape.warnings() approach is well conceived and does a good job.
Thank you!

[davidmccabe]

We upgraded Cytoscape in our app recently and now it always spews thousands of warnings to the console, making the console useless for seeing actually important warnings. Your solution is to allow the option of disabling all warnings across Cytoscape -- this seems a much worse tradeoff than just not warning about overlapping nodes. What if Cytoscape needs to warn be about something else?

[maxkfranz]

The edge warning is the most important warning that the library emits, for reasons already described in this thread.

The library doesn't print warnings for overlapping nodes anymore --- as mentioned.

If you have that warning, especially that many times, then that's a good indication that you have problem in your stylesheet. If you don't notice the issue right away by eye, then that makes a good case for the existence of the warning. How else are you supposed to notice something missing in large graphs?