Conversation
|
@Ma77Ball Please review it first. |
Ma77Ball
left a comment
Ma77Ball
left a comment
There was a problem hiding this comment.
The code looks overall good. I left a few comments and noticed a failing test case. I would recommend running the test cases locally on your GitHub and identifying the issue.
There was a problem hiding this comment.
Please change this image to be a valid png with no background.
| @JsonProperty(value = "fillOpacity", required = true) | ||
| @JsonSchemaTitle("Fill Opacity") | ||
| @JsonPropertyDescription( | ||
| "Opacity value for radar chart fill from 0.0 (transparent) to 1.0 (opaque)" | ||
| ) | ||
| @JsonSchemaInject(json = """{ "minimum": 0.0, "maximum": 1.0 }""") | ||
| var fillOpacity: Double = 0.5 |
There was a problem hiding this comment.
Make the default value show on the frontend. Also, it would be better to use 0.1 increments for opacity instead of 1.
| assert(nameColumn.nonEmpty && valueColumns != null && !valueColumns.isEmpty) | ||
| val valueColsList = valueColumns.mkString("', '") | ||
| s""" | ||
| | required_cols = ['$nameColumn', '$valueColsList'] | ||
| | table.dropna(subset=required_cols, inplace=True) | ||
| | # Ensure all value columns are numeric | ||
| | value_cols = ['$valueColsList'] |
There was a problem hiding this comment.
Causes an error when there is an apostrophe in the column name. I recommend substituting apostrophes in column names to \'
| val valueColsList = valueColumns.mkString("', '") | ||
| s""" | ||
| | # Create radar chart | ||
| | fig = go.Figure() | ||
| | categories = ['$valueColsList'] |
There was a problem hiding this comment.
Same as valueColsList above.
There was a problem hiding this comment.
I have made all the mentioned changes. when I ran 'sbt scalafixAll', a few scala files (not related to my radar chart) were changed - are these changes specific to my local environment or should I push these changes as well?
There was a problem hiding this comment.
The changes should be specific to your local environment unless introduced from a previous pr. I recommend pushing the changes along with sbt scalafmtAll for review. (Although I'm not sure why yours would be different from the other Prs being raised)
There was a problem hiding this comment.
Or should I just push my changes alone for now and leave the 'sbt scalafixAll' changes to my local?
There was a problem hiding this comment.
I recommend pulling and merging the upstream main repo into your repo. After checking the diff between your repo and the Texera main repo to see whether those changes are already present, or if a separate PR needs to be raised to fix the issue. Also, make sure your versions of each software match the wiki.
There was a problem hiding this comment.
I might also recommend, since not many files were changed, that you make your main match the upstream Apache/Texera repo and create a fork of the original repo with your changes. I'm not sure why your files are different.
There was a problem hiding this comment.
Yeah understood, I'll be match my main with the upstream texera repo and then update my changes
|
In the future, please respond to the comments and mention a little of what you did to address the issue after you can hit resolve comment at the bottom of each. This action helps with the review process. |
3 participants
What changes were proposed in this PR?
This change introduces a radar chart operator, which visualizes multivariate data by plotting multiple quantitative variables on axes that radiate from a central point.
In a radar chart:
The operator takes as input a set of variables (features) for each observation and outputs a radar chart that enables easy visual comparison across all provided dimensions.
Any related issues, documentation, discussions?
No related issues, documentation, discussions have been linked to this feature at this time.
How was this PR tested?
A dedicated test file (TestRadarChart.scala) was created to thoroughly test the radar chart operator, covering both typical and edge cases. The tests verify correct rendering and data handling for various input scenarios. In addition, all existing test cases were run using sbt test to ensure no regressions were introduced. Manual testing was also performed in the Texera UI to visually confirm the radar chart output.
Was this PR authored or co-authored using generative AI tooling?
This PR was co-authored by Perplexity AI.