-
-
Notifications
You must be signed in to change notification settings - Fork 994
feat: add base interface of Unicode plot and add implementation of bar plot
#5444
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
---
type: pre_push_report
description: Results of running various checks prior to pushing changes.
report:
- task: run_javascript_examples
status: na
- task: run_c_examples
status: na
- task: run_cpp_examples
status: na
- task: run_javascript_readme_examples
status: na
- task: run_c_benchmarks
status: na
- task: run_cpp_benchmarks
status: na
- task: run_fortran_benchmarks
status: na
- task: run_javascript_benchmarks
status: na
- task: run_julia_benchmarks
status: na
- task: run_python_benchmarks
status: na
- task: run_r_benchmarks
status: na
- task: run_javascript_tests
status: na
---
… plot
---
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes.
report:
- task: lint_filenames
status: passed
- task: lint_editorconfig
status: passed
- task: lint_markdown
status: na
- task: lint_package_json
status: na
- task: lint_repl_help
status: na
- task: lint_javascript_src
status: passed
- task: lint_javascript_cli
status: na
- task: lint_javascript_examples
status: na
- task: lint_javascript_tests
status: na
- task: lint_javascript_benchmarks
status: na
- task: lint_python
status: na
- task: lint_r
status: na
- task: lint_c_src
status: na
- task: lint_c_examples
status: na
- task: lint_c_benchmarks
status: na
- task: lint_c_tests_fixtures
status: na
- task: lint_shell
status: na
- task: lint_typescript_declarations
status: na
- task: lint_typescript_tests
status: na
- task: lint_license_headers
status: passed
---
---
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes.
report:
- task: lint_filenames
status: passed
- task: lint_editorconfig
status: passed
- task: lint_markdown
status: na
- task: lint_package_json
status: na
- task: lint_repl_help
status: na
- task: lint_javascript_src
status: na
- task: lint_javascript_cli
status: na
- task: lint_javascript_examples
status: na
- task: lint_javascript_tests
status: na
- task: lint_javascript_benchmarks
status: na
- task: lint_python
status: na
- task: lint_r
status: na
- task: lint_c_src
status: na
- task: lint_c_examples
status: na
- task: lint_c_benchmarks
status: na
- task: lint_c_tests_fixtures
status: na
- task: lint_shell
status: na
- task: lint_typescript_declarations
status: na
- task: lint_typescript_tests
status: na
- task: lint_license_headers
status: passed
---
---
type: pre_push_report
description: Results of running various checks prior to pushing changes.
report:
- task: run_javascript_examples
status: na
- task: run_c_examples
status: na
- task: run_cpp_examples
status: na
- task: run_javascript_readme_examples
status: na
- task: run_c_benchmarks
status: na
- task: run_cpp_benchmarks
status: na
- task: run_fortran_benchmarks
status: na
- task: run_javascript_benchmarks
status: na
- task: run_julia_benchmarks
status: na
- task: run_python_benchmarks
status: na
- task: run_r_benchmarks
status: na
- task: run_javascript_tests
status: na
---
|
/stdlib update-copyright-years |
|
This pr is Intentionally incomplete , i kind of need acknowledgement weather i can go ahead with this or not, I will add the detail detail description which explain this pr . |
PALLETLow level Class for unicode/ansi plot This class maintains a 2D array of characters, colors, and background colors. During the rendering process, these elements are converted into their respective ANSI escape codes. FeaturesThis class provides methods to manipulate the 2D array, including:
PlotBaseOverview
Drawing Fundamental StructuresThe core idea behind drawing each structure is simple:
Example WalkthroughDrawing a Rectangle
drawRectangle( [ 0, 3 ], [ 1, 3 ] ); // Height: (0,3), Width: (1,3)
setNonEnumerableReadOnly( Pallete.prototype, 'setcanvas', function setcanvas() {
var remainwidth = process.stdout.columns - 0;
var background;
var canva = '';
var color;
var i;
var j;
for ( i = this._sketch.length - 1; i >= 0; i-- ) {
canva += '\r';
for ( j = 0; j < this._sketch[ 0 ].length && j < remainwidth; j++ ) {
// If background color is not same as previous then only we need to change it
if ( !this.isLegal( i, j - 1 ) || this._backgrounds[ i ][ j ] !== this._backgrounds[ i ][ j - 1 ] ) {
background = ansiColors[ this._backgrounds[ i ][ j ] ];
canva += background;
}
// If forward color is not same as previous then only we need to change it
if ( !this.isLegal( i, j - 1 ) || this._colors[ i ][ j ] !== this._colors[ i ][ j - 1 ] ) {
color = ansiColors[ this._colors[ i ][ j ] ];
canva += color;
}
canva += markers[ this._sketch[ i ][ j ] ];
}
canva += ansiColors[ 'def' ];
canva += '\n';
}
this.canva = canva;
return canva;
} );every type of plot use this base plot to draw ther visulization all scaling work related to the cordinates also done in the specific plot class barPlot
Questions
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the directory "static_plots"? Is there a reason to distinguish between "static" and presumably "dynamic"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actully when i see the sparkline code base i thought it take the dynamic inputs means continues from buffer , so the interface i have designe is for static plots only in which we provide full date before rendering , so thats the reason why i have put the name static_plot .
| out.cols = void 0; | ||
| out.xaxis = []; | ||
| out.yaxis = []; | ||
| out.barColor = 'Fbl'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the color naming scheme here? "Fbl" is not immediately obvious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fbl stands for forward ( it is name i have seen used in ANSI encoding ) color black , we also have the Bbl for background black . we can change the color schema naming afterwards.
| }; | ||
| var inst = new barchart(options); | ||
|
|
||
| inst.draw(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the distinction between "draw" and "render"?
| var getLabelColor = require( './props/labelColor/get.js' ); | ||
| var setMarker = require( './props/marker/set.js' ); | ||
| var getMarker = require( './props/marker/get.js' ); | ||
| var primitive = require( './../base/lib' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No walking directories like this. This is inherently fragile if working to access the functionality of another package. If an API does not exist in a package, always use package identifiers.
| var getRows = require( './props/rows/get.js' ); | ||
| var setCols = require( './props/cols/set.js' ); | ||
| var getCols = require( './props/cols/get.js' ); | ||
| var setBarColor = require( './props/barColor/set.js' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Folder names should always be snakecase, never camelcase. barColor => bar-color.
| 'value': null | ||
| }); | ||
| } | ||
| defineProperty( this, '_primitive', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we do this rather than inherit from a "base" class?
|
|
||
| finalCordx = []; | ||
| finalCordy = []; | ||
| this.addLableX(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo. addLabelX.
| /** | ||
| * Add x label and tics. | ||
| * | ||
| * @name addLableX |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typos.
| * @name addCategory | ||
| * @memberof Primitive.prototype | ||
| * @type {Function} | ||
| * @returns {void} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to document parameters.
|
|
||
| /** | ||
| * @name rows | ||
| * @memberof Primitive.prototype |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/Primitive/BarPlot/ here and elsewhere.
| * @throws {TypeError} | ||
| * | ||
| */ | ||
| defineProperty( BarPlot.prototype, 'rows', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does a bar chart have the concept of rows and columns?
| * @throws {TypeError} | ||
| * | ||
| */ | ||
| defineProperty( BarPlot.prototype, 'barColor', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if I want bars to have different colors?
| * @throws {TypeError} | ||
| * | ||
| */ | ||
| defineProperty( BarPlot.prototype, 'labelColor', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if I want labels to have different colors?
| * @param {number} containerWidth - width of the container | ||
| * @returns {Array} array of scale represent width and height of bar | ||
| * */ | ||
| function scaleBar( x, y, containerWidth ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Helper functions such as this should be moved to the top of the file in a // FUNCTIONS // section before // MAIN //.
| * Returns the rendering mode. | ||
| * | ||
| * @private | ||
| * @returns {Array} rendering mode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is incorrect.
| * Sets the rendering mode. | ||
| * | ||
| * @private | ||
| * @param {string} color - indicate the color code. | ||
| * @throws {TypeError} must be a number |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is incorrect.
|
|
||
| // VARIABLES // | ||
|
|
||
| var colors = require( '@stdlib/plot/unicode/static_plots/marker/colors.js' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't reach into packages like this. Find another way. If, in the future, we rename the colors.js file, this will break here, but we cannot possibly know that beforehand because you pierced the package wall. Files within a package should always be considered private.
| */ | ||
| function set( color ) { | ||
| /* eslint-disable no-invalid-this */ | ||
| if ( !( color in colors ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use in. Use, e.g., array/base/assert/contains.
| * Returns the rendering mode. | ||
| * | ||
| * @private | ||
| * @returns {Array} rendering mode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect.
| return; | ||
| } | ||
| if ( !isNumber( num ) ) { | ||
| throw new TypeError( format( 'invalid assignment. must be a array.' ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Must be an array and yet we are checking for a number.
| * Returns the rendering mode. | ||
| * | ||
| * @private | ||
| * @returns {Array} rendering mode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect.
| */ | ||
| function get() { | ||
| /* eslint-disable no-invalid-this */ | ||
| return this._cols; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we returning _cols when this is the marker property?
|
|
||
| // VARIABLES // | ||
|
|
||
| var markers = require( '@stdlib/plot/unicode/static_plots/marker/markers.js' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, why are all these files in bar/*? Why are they not in bar/lib/*?
| // VARIABLES // | ||
|
|
||
| var defaults = { | ||
| 'backgroundColor': 'Bwh', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to explain the color naming convention here.
|
|
||
| var defaults = { | ||
| 'backgroundColor': 'Bwh', | ||
| 'forwardColor': 'Frd', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presumably, by forward you mean foreground, correct?
| var defaults = { | ||
| 'backgroundColor': 'Bwh', | ||
| 'forwardColor': 'Frd', | ||
| 'marker': 'sp' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is sp?
| // MAIN // | ||
|
|
||
| /** | ||
| * Pallete constructor. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this class, I don't have a good idea what is meant by "Pallete" and what this class is useful for.
| }); | ||
|
|
||
| /** | ||
| * Display the rendered plots. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have many methods for "rendering". Why build, show, draw, drawFrame, draw*, and render (elsewhere)? It is not clear how these all fit together and why it makes sense to organize the code like this.
| }); | ||
|
|
||
| /** | ||
| * @name setSize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presumably this should be size. You also need to add descriptions.
|
|
||
| 'use strict'; | ||
|
|
||
| var colors = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notice the documentation in this file:
| var ANSI_ESCAPE_CODES = { |
| '[1,1,1,1]': '█' | ||
| }; | ||
|
|
||
| for ( i = 0; i < 10; i++ ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the point of this loop?
|
Questions/comments:
|
Thanks again for the guidance—I’ll iterate based on this feedback! |
|
/stdlib help |
|
@Vinit-Pandit, available slash commands include:
|
|
/stdlib rebase |
@Vinit-Pandit, the slash command failed to complete. Please check the workflow logs for details. |
|
I'll add the new PR , it's already too messed up. |
|
Opened new pr #6750 |


Progress #2010
Description
This pull request:
unicodeplot and implement thebar plotRelated Issues
This pull request:
Questions
Yes
Other
No.
Checklist
@stdlib-js/reviewers
unicode bar plot
line char ( under construcution )