Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#14583 closed defect (fixed)

[patch][cla]dojox.charting.spiderchart problem with drawing one polygon

Reported by: Martin Repta Owned by: cjolif
Priority: low Milestone: 1.8
Component: Charting Version: 1.7.1
Keywords: dojox, charting, spiderchart Cc: martinerko@…
Blocked By: Blocking:

Description

Hi,

it seems that spiderchart has problem to draw chart with only one polygon. There is big issue with dividing by zero. See line 341 here:https://github.com/dojo/dojox/blob/master/charting/plot2d/Spider.js

I have discussed this problem here and stated also sample http://dojo-toolkit.33424.n3.nabble.com/dojox-charting-spiderchart-td3632351.html

Attachments (1)

spiderchart.patch (810 bytes) - added by Martin Repta 8 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 8 years ago by Adam Peller

Component: DojoxCharting
Owner: changed from Adam Peller to cjolif
Priority: highestnormal
severity: normalmajor

comment:2 Changed 8 years ago by cjolif

Milestone: tbdfuture
severity: majornormal
Status: newassigned

There is a workaround, so I don't think this is a major issue. Fixing it requires introducing new API, because surprisingly the component has not been designed for that use-cases...

Changed 8 years ago by Martin Repta

Attachment: spiderchart.patch added

comment:3 Changed 8 years ago by Martin Repta

I have just fixed this bug. Patch file is attached.

Martin

comment:4 Changed 8 years ago by Adam Peller

Summary: dojox.charting.spiderchart problem with drawing one polygon[patch][cla]dojox.charting.spiderchart problem with drawing one polygon

comment:5 Changed 8 years ago by cjolif

Martin many thanks for you patch and your time to contribute it.

My comments:

What your patch is doing is that it takes all the values of the various series of the spider chart and make the overall min value the min value for all the series and the overall max value the max value for all the series.

In some cases that might be what people want but in a lot of cases the various series of the spider chart will display values on totally different scales (for example population of a country in one series and GDP growth in another). In this case you patch that will not give a correct result.

The ideal solution would be a new "API" to let the user choose the min and max per series. That API would also be useful with multiple series use-cases and you don't always want to pick automated computed values.

For example

addSeries("MySeries", { data: { ... }, min: , max }, { fill : "geen" });

That would not prevent also having the mechanism you coded but it can't be the only one for the reasons I stated above.

If you could rework the patch with that idea in mind I think it would make a great addition.

Last edited 8 years ago by cjolif (previous) (diff)

comment:6 Changed 8 years ago by Martin Repta

I can rewrite my patch, but do not forget, my current fix is applied, when chart has just one series, so there should not be problem with scales of different series, because they do not exist. Do you understand what I mean?

comment:7 Changed 8 years ago by Martin Repta

can you answer please?

comment:8 Changed 8 years ago by cjolif

marineko, sorry but no I don't get what you meant. Even with a single series in a spider chart (that is a specific case != from cartesian chart) there is a "scale" per item in the series. For example:

data : { "GDP" : 10, "Population": 100 } 

there are 2 potential scale one for GDP and one for population even if I have a single series.

Do you see my point?

Last edited 8 years ago by cjolif (previous) (diff)

comment:9 Changed 8 years ago by Martin Repta

Sorry, I do not understand what you mean? Can you provide a sample?

comment:10 in reply to:  9 Changed 8 years ago by cjolif

Replying to martinerko:

Sorry, I do not understand what you mean? Can you provide a sample?

Ok let's say my chart represents two values country population (in million) and GDP growth in percentage.

The first value will usually range from 1 to let's say 1000. The second value will range from 0 to let's say 10.

So I want the min for population to be 1 and the max 1000. While I want to have the min for GDP growth to be 0 and the max 10.

With your patch with the example I put above:

data : { "GDP" : 10, "Population": 100 } 

I will get GDP growth axis ranging from 10 to 100 and population axis ranging from 10 to 100 as well. Which is not what I want based on my description below?

Am I clearer?

One more time I agree that your patch is certainly solving your particular issue but I would like to see something more open to cover other use-cases as the one I described above.

Thanks.

comment:11 Changed 8 years ago by cjolif

Milestone: future1.8
Priority: highlow

Someone else needs the wider solution. See: http://stackoverflow.com/questions/8966268/fixed-axes-in-dojo-spider-chart/8975782#8975782

I'll try to come up with something in 1.8 about that.

comment:12 Changed 8 years ago by cjolif

#14738 is a duplicate of this ticket.

comment:13 Changed 8 years ago by cjolif

Resolution: fixed
Status: assignedclosed

In [27862]:

fixes #14583. This is done by introducing axis concept for spider. User can set a min & a max for the axis. For that some refactoring has been done by:

  • creating a CartesianBase? plot that cartesian chart (all but Pie & Spider) inherits instead of directly Base plot
  • getting Spider plot implements Axis methods from Base
  • ability to set min/max properties on Base axis to be used with Spider plots
  • small clean ups here & there.

!strict.

comment:14 Changed 8 years ago by cjolif

In [27864]:

refs #14583. Missing Pie modification commit. !strict.

comment:15 Changed 8 years ago by cjolif

In [27865]:

refs #14583. Fix grid regression introduced by [27862]. !strict.

comment:16 Changed 8 years ago by cjolif

You will to do something like the following to specify min/max of the axis:

chart.addAxis("GDP", {type: "Base", min: 0, max: 50 });

comment:17 Changed 8 years ago by cjolif

In [27898]:

refs #14583. Even after plot refactoring some plots might still not extend plot2d/Base. So make sure we are still compatible with plots not inheriting from Base. In the future we might have Base for all plots and SeriesBase? for series based plots (i.e. ones that plot series not something else like Grids). !strict.

comment:18 Changed 8 years ago by cjolif

In [28286]:

refs #14583. Fix issue on missing axis for pie with DataChart. !strict.

Note: See TracTickets for help on using tickets.