Feat/piechart #168

Tanguylo · 2022-06-13T09:11:07Z

Start with a piechart with mouse over managing

…ing parts

plot_data/core.py

plot_data/templates.py

script/pie_chart.py

src/primitives.ts

src/subplots.ts

src/plot-data.ts

…ull request

plot_data/core.py

src/primitives.ts

xavier-ferry · 2022-06-22T08:47:21Z

src/primitives.ts

+  lists: any[]=[];
+  to_display_attributes: Attribute[]=[];
+
+  constructor(public tooltip:Tooltip,


Can remove tooltip
Can type_ be removed from the constructor ?

src/primitives.ts

src/plot-data.ts

src/primitives.ts

…r practical reasons

… to Point2D

src/primitives.ts

src/subplots.ts

src/primitives.ts

src/subplots.ts

…hanges following the PR review

src/subplots.ts

…nymore PlotPieChart's one and change radius to have a standard PieChart instantiated in plots with a .transform(matrix)

xavier-ferry · 2022-06-28T13:14:17Z

src/primitives.ts

+  path: Path2D;
+  clicked: boolean = false;
+  selected: boolean = false;
+  points_inside: PiePart[] = [this];


It is really strange to have this kind of things.
This is a first way to handle the mouse. A better implementation, via a MouseHandler is the purpose of a future dev (#172).

I suggest that we keep the code in commit (174b386) as a first implementation

xavier-ferry

As said in comments, some wip related to mouse handling is already implemented here but it does not prevent me from approving this PR

GhislainJ

I didn't perform the review for most of the subplot.ts file as the diff was horrendous and changes were mostly convention stuff

GhislainJ · 2023-03-01T13:27:21Z

CHANGELOG.md


 ## Unreleased => [0.7.2]
+### Add
+- Add Piechart drawing


Should update changelog to set it in the right version (ie. current)

GhislainJ · 2023-03-01T13:30:08Z

plot_data/core.py

+                 name: str = ''):
+
+        self.slicing_variable = slicing_variable
+        if not data_samples:


if data_samples is None: would be the right check

GhislainJ · 2023-03-01T13:31:07Z

plot_data/templates.py

  </body>
 </html>
 ''')
+


Is this template any different than the cypress' one ? Can't they be mutualized ? Should they ?

GhislainJ · 2023-03-01T13:32:26Z

src/multiplots.ts

        }
        this.initializeObjectContext(newObject);
        this.objectList.push(newObject);
+        console.log(newObject)


Intempestive log ?

GhislainJ · 2023-03-01T13:33:23Z

src/primitives.ts

 import { EdgeStyle, SurfaceStyle, PointStyle, TextStyle } from "./style";
 import { set_default_values, genColor, drawLines, getCurvePoints, Tooltip, Axis, PointFamily, Attribute, TypeOf } from "./utils";
 import { Shape, List, MyObject } from "./toolbox";
+import { ContextExclusionPlugin } from "webpack";


Unused import ? Most probably due to VS Code auto import feature

GhislainJ · 2023-03-01T13:36:39Z

src/toolbox.ts

@@ -1,3 +1,4 @@
+import { ListenOptions } from "net";


Auto vs code import ?

GhislainJ · 2023-03-01T13:43:28Z

src/plot-data.ts

    this.latest_selected_points_index = [];
-    for (let i=0; i<this.latest_selected_points.length; i++) {
+    if (this.plotObject instanceof PieChart) {
+      elements_list = this.plotObject.pieParts;


Future : To keep in mind while refactoring : In order to avoid these kind of if statements everywhere, as any "plotObject" should inherit from a base class, we should define a getter (js equivalent of a python property), that would enable high customizability when we call the getter as well as guarantee simplicity and genericity.

For ex, here, the base class would define the getter as return this.point_list where PieChar would overwrite this behavior to call and yield its own "way" of representing itself, ie. return this.pieParts

And the code would only call for the getter

GhislainJ · 2023-03-01T13:45:54Z

src/plot-data.ts

          this.plotObject.graphs[i].point_list[j].selected = false;
          this.plotObject.graphs[i].point_list[j].clicked = false;
        }
      }
-    }
+    } else if (this.plotObject instanceof PieChart) {


Exact same comment as the previous one here, but with a function.

Globally speaking, wherever there are a lot of "if/else if/else" statements, there is almost everytime a better way to do it by using Oriented Object stuff.

See dc's schema refactor for an example

GhislainJ · 2023-03-01T13:50:00Z

src/subplots.ts

-            }
+  plot_datas: any;
+  selected: boolean = true;
+  public constructor(public data: any,


[Wait for refactor] Be aware of JS Indentation Conventions here

GhislainJ · 2023-03-01T13:53:49Z

I let some comments even if I approve the PR in order to avoid blocking stuff

Tanguylo added 6 commits June 3, 2022 14:51

Ajout class Pie

e2280d1

Start with piechart

d83ec07

feat(piechart): first piechart drawing

d5b2e4b

feat(piechart): add colors to piechart

b4514ea

feat(piechart):add pie part surface coloring on mouse over

e21e984

feat(piechart):partial code cleaning

949df9c

Tanguylo marked this pull request as draft June 13, 2022 09:12

Tanguylo added 7 commits June 13, 2022 12:19

feat(piechart):fix a git mistake with branches

8427fef

feat(piechart):fixing last diff in core.py

e21259e

feat(piechart):remove ' /' line 931

ad59106

feat(piechart):add parts selection on click

6c494e2

feat(piechart):manage piepart paths as piepart attribute

9c83b56

feat(piechart):initialize pieparts path when instantiating when creat…

38ebcb3

…ing parts

feat(piechart):some code cleaning

1937904

Tanguylo marked this pull request as ready for review June 17, 2022 07:54

Tanguylo requested review from GhislainJ, Jeremie-Ch and xavier-ferry and removed request for Jeremie-Ch June 17, 2022 07:55

xavier-ferry suggested changes Jun 20, 2022

View reviewed changes

Tanguylo added 4 commits June 21, 2022 16:08

feat(piechart): fix mistakes following the first 6 comments on this p…

4675ede

…ull request

feat(piechart): fix the 5 following remarks of the PR

cd1864f

feat(piechart): fix the remaining remarks except the drawPieChart method

1f81acc

feat(piechart): take last PR remarks into account

e3371ec

xavier-ferry suggested changes Jun 22, 2022

View reviewed changes

src/plot-data.ts Outdated Show resolved Hide resolved

src/plot-data.ts Outdated Show resolved Hide resolved

src/plot-data.ts Outdated Show resolved Hide resolved

src/plot-data.ts Outdated Show resolved Hide resolved

src/primitives.ts Outdated Show resolved Hide resolved

Tanguylo added 2 commits June 22, 2022 16:11

feat(piechart): change type of color_to_plot_data to map

84fe419

feat(piechart): modifications following PR remarks

8bf219a

Tanguylo linked an issue Jun 23, 2022 that may be closed by this pull request

Piechart drawing #170

Open

1 task

feat(piechart): Changes following the PR review

006de63

Tanguylo added 3 commits June 24, 2022 11:03

feat(piechart): Put color:string='' in the constructor of PieParts fo…

3cea6f1

…r practical reasons

feat(piechart_in_multiplot): Handle selection of piePart as expected

174b386

feat(piechart): add commit to branch and reset latest_selected_points…

0413f2f

… to Point2D

xavier-ferry suggested changes Jun 27, 2022

View reviewed changes

Tanguylo added 5 commits June 27, 2022 10:37

feat(piechart): Undo color_to_plot_data as a Map

87d5ef6

feat(piechart): Changes following second PR

10772ba

feat(piechart): DataSample and DataSamples type definition and some c…

34cdd69

…hanges following the PR review

feat(piechart): Changes in drawPieChart

7a5f0e6

feat(piechart): Change multiple_plot.py file

54091b1

xavier-ferry suggested changes Jun 28, 2022

View reviewed changes

src/subplots.ts Outdated Show resolved Hide resolved

feat(piechart): Call definePieParts in PieChart constructor and not a…

0ecc993

…nymore PlotPieChart's one and change radius to have a standard PieChart instantiated in plots with a .transform(matrix)

xavier-ferry reviewed Jun 28, 2022

View reviewed changes

xavier-ferry approved these changes Jun 28, 2022

View reviewed changes

Tanguylo and others added 6 commits June 28, 2022 17:04

feat(piechart): Write PR equivalent in CHANGELOG.md file

5c7c296

Merge branch 'dev' into feat/piechart

b4bc6e2

merge since 6 months, possible fails

ce09723

Merge branch 'dev' into feat/piechart

9896338

feat(piechart): add tests

2187adf

feat(piechart): pylint

f8f4b8a

GhislainJ approved these changes Mar 1, 2023

View reviewed changes

Tanguylo added the status: stopped label Jun 9, 2023

Feat/piechart #168

Are you sure you want to change the base?

Feat/piechart #168

Uh oh!

Conversation

Tanguylo commented Jun 13, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xavier-ferry left a comment

Choose a reason for hiding this comment

Uh oh!

GhislainJ left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GhislainJ commented Mar 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants