core icon indicating copy to clipboard operation
core copied to clipboard

Better nvd3 graph extents in Reporting

Open Roy-Orbison opened this issue 10 months ago • 3 comments

  • [x] I have read the contributing guide lines at https://github.com/opnsense/core/blob/master/CONTRIBUTING.md
  • [x] I am convinced that my issue is new after having checked both open and closed issues at https://github.com/opnsense/core/issues?q=is%3Aissue

Is your feature request related to a problem? Please describe.

This issue has two intertwined parts:

  1. Graphed values can exhibit dramatic visual variance when in fact they are quite close, so do not warrant the dramatic appearance (at least not by default). They can also be misleading in that some values appear low because they are close to the lower extent of the graph, but are high in absolute terms.
  2. Graph extents are cropped at whatever the min & max. values are, which is less clear than if they are rounded to sensible multiples so that top and bottom lines are shown, and the divisions are uniform.

Describe the solution you'd like

  1. Add an option toggle (just like the Inverse one) to the graph UI that forces the Y-axis to anchor the scale to 0. Ideally it would be on by default. The code for this is small:

    chart.forceY([Math.min(0, min), Math.max(0, max)]);
    

    where min and max are already known or obtained from d3.extent(). I think it can be reverted with this:

    chart.forceY([]);
    
  2. Round the extents to a sensible multiple so that the horizontal lines are evenly spaced. With a function like this:

    function sensibleExtents(min, max) {
    	const largestMagnitude = Math.max(Math.abs(min), Math.abs(max));
    	// largest power of 10 below the largest magnitude
    	let division = Math.pow(10, Math.ceil(Math.log10(largestMagnitude)) - 1);
    	// extra clamping to prevent wasted space when magnitude is barely larger than division, e.g. an extent of 200 on a max. of 105
    	if (largestMagnitude < (1.5 * division)) {
    		division /= 10;
    	}
    	return [
    		division * Math.floor(min / division),
    		division * Math.ceil(max / division)
    	];
    }
    

    we get sensibleExtents(-12345, 432) -> [-13000, 1000], and sensibleExtents(0.05, 0.18) -> [0, 0.2].

    Combining that with (1):

    chart.forceY(sensibleExtents(Math.min(0, min), Math.max(0, max)));
    

    and

    chart.forceY(sensibleExtents(min, max));
    

Describe alternatives you considered

A personal JavaScript bookmarklet that adjusts the graphs for me, but would not benefit others.

Additional context

Possibly related to #6527, but that is about configurable extents, this is about sensible, dynamic defaults.

Compare these examples of a set with automatic extents vs forced: image

image

Roy-Orbison avatar Apr 19 '24 04:04 Roy-Orbison

@Roy-Orbison if you feel up to to the task, would you mind opening a pull request?

AdSchellevis avatar Apr 19 '24 06:04 AdSchellevis

Possibly. I was hoping to avoid setting up a build environment/VM for a small JS change. Maybe I could have a go and someone who knows that code could polish it up?

Roy-Orbison avatar Apr 19 '24 07:04 Roy-Orbison

if you start with a PR, I'll review and see if we can polish it a bit. sure

AdSchellevis avatar Apr 19 '24 08:04 AdSchellevis