Circleci project setup - Githubissues

zackbakerusc commented 4 years ago

This version includes previous overlay work but passes flake8 and pytest

lgtm-com[bot] commented 4 years ago

This pull request introduces 9 alerts when merging fe5ed6ff12e2013bfb6f9b1656e1f6967271796e into 41f50df2f5b499425465f506a9aae5acf1a39c0b - view on LGTM.com

new alerts:

7 for Wrong name for an argument in a call
1 for Unused local variable
1 for Module is imported with 'import' and 'import from'

lgtm-com[bot] commented 4 years ago

This pull request introduces 1 alert when merging 2a69cd9a4c5b6ff5d1f3260085b3e849fa3f923a into 41f50df2f5b499425465f506a9aae5acf1a39c0b - view on LGTM.com

new alerts:

1 for Module is imported with 'import' and 'import from'

lgtm-com[bot] commented 4 years ago

This pull request introduces 1 alert when merging e891d903059381911fc8dd1bb5b173946a6f4e0e into 41f50df2f5b499425465f506a9aae5acf1a39c0b - view on LGTM.com

new alerts:

1 for Module is imported with 'import' and 'import from'

zackbakerusc commented 4 years ago

Victoriously passing all flake8, pytest, and coveralls testing..

zackbakerusc commented 4 years ago

None of that should be a problem. I'll get on it this weekend.

On Wed, Apr 1, 2020 at 8:50 AM Tammo Ippen notifications@github.com wrote:

@tammoippen requested changes on this pull request.

Sorry for the late review. This is a initial review of the stuff that directly comes to my mind. I still want to play around with some of the features.

In .circleci/config.yml https://github.com/tammoippen/plotille/pull/31#discussion_r401595168:

@@ -121,25 +121,6 @@ jobs:
 steps:

   - tester
Please revert all changes in this file.

In .travis.yml https://github.com/tammoippen/plotille/pull/31#discussion_r401595309:

@@ -0,0 +1,3 @@

+language: python

Remove file.

In examples/example_ellipse.py https://github.com/tammoippen/plotille/pull/31#discussion_r401596006:

@@ -0,0 +1,32 @@

+import plotille

+

+

+if name == "main":

Let's have consistent examples and not use the if name == "main": pattern.

In examples/example_kmeans.py https://github.com/tammoippen/plotille/pull/31#discussion_r401597440:

@@ -0,0 +1,36 @@

+import numpy as np

+

+from sklearn.cluster import KMeans

+from sklearn.datasets import make_blobs

+from skimage.measure import EllipseModel

Can you add a requirements.txt in examples/ with all dependencies needed in order to run the examples, e.g. plotille, numpy, sklearn....

This way, somebody that wants to try the examples can just do a pip install -r requirements.txt and run the examples.

In examples/example_scatter_cats.py https://github.com/tammoippen/plotille/pull/31#discussion_r401598042:

@@ -0,0 +1,35 @@

+import plotille

+

+# may need: export PYTHONIOENCODING=utf8

instead, please add a

-- coding: utf-8 --

as the first line of the example.

In examples/example_scatter_cats.py https://github.com/tammoippen/plotille/pull/31#discussion_r401598466:

names.append("MOUSE %d" % (mouse_index))

+

fig.scatter(listy, listx, lc='red', label='MOUSE', marker='o', text=names)

listx = []

listy = []

for cat_index, cat in enumerate(cat_list):

listx.append(cat[0])

listy.append(cat[1])

fig.scatter(listy, listx, lc='green', label='Cat', marker='x', text='Cat')

print(fig.show(legend=True))

+if name == "main":

same: no if name == "main"

In examples/example_single_plot.py https://github.com/tammoippen/plotille/pull/31#discussion_r401599172:

@@ -0,0 +1,4 @@

+import plotille

+import numpy as np

+X = np.linspace(0, 2*np.pi, 20)

can you run flake8 on the example folder as well and fix the issues?

In examples/example_stocks.py https://github.com/tammoippen/plotille/pull/31#discussion_r401599598:

@@ -0,0 +1,55 @@

+import pandas as pd

+import pandas_datareader as web

+import datetime

+

+import requests_cache

why requests_cache and nor requests?

In examples/run_all_examples.sh https://github.com/tammoippen/plotille/pull/31#discussion_r401600107:

@@ -0,0 +1,8 @@

+for f in .py; do # or wget-.sh instead of *.sh

Instead of this file, can you create a README.md explaining how to run the examples?

In plotille/_canvas.py https://github.com/tammoippen/plotille/pull/31#discussion_r401657351:

@@ -147,9 +157,18 @@ def _set(self, x_idx, yidx, set=True, color=None):
     y_c, y_p = y_idx // 4, y_idx % 4

     if 0 <= x_c < self.width and 0 <= y_c < self.height:
self._canvas[y_c][x_c].update(x_p, yp, set)

if color:

self._canvas[y_c][x_c].fg = color

if(not overlay):

No parentheses here and in all other if places, if not necessary.

In plotille/_canvas.py https://github.com/tammoippen/plotille/pull/31#discussion_r401659979:
     """
return linesep.join(''.join(map(six.text_type, row))

for row in reversed(self._canvas))

classic:

Remove legacy comments.

In plotille/_canvas.py https://github.com/tammoippen/plotille/pull/31#discussion_r401662061:
     """
return linesep.join(''.join(map(six.text_type, row))

for row in reversed(self._canvas))

classic:

ret = linesep.join(''.join(map(six.text_type, row)) for row in reversed(self._canvas))

return ret

combine _canvas (Dots) and overlay (text) so that overlay is on top of dots

ret_combined = ''

for row_index in range(len(self._canvas))[::-1]:

for ii in range(len(self._canvas[row_index])):

if(self._canvas_overlay[row_index][ii] == ' '):

This means, if the text in the overlay has a ' ', we might have dots between the text?

In plotille/_figure.py https://github.com/tammoippen/plotille/pull/31#discussion_r401664295:

@@ -28,6 +28,7 @@

from itertools import cycle

import os

+import numpy as np

This is not a dependency of plotille and i would like to keep it that way. Please implement with stdlib funktions.

In plotille/_figure.py https://github.com/tammoippen/plotille/pull/31#discussion_r401665782:

def scatter(self, X, Y, lc=None, label=None): # noqa: N803

def scatter(self, X, Y, lc=None, label=None, marker='', text=None): # noqa: N803

Better use None to indicate the field is not used? ⬇️ Suggested change

def scatter(self, X, Y, lc=None, label=None, marker='', text=None): # noqa: N803

def scatter(self, X, Y, lc=None, label=None, marker=None, text=None): # noqa: N803

The test later can be something like: if marker:

In plotille/_figure.py https://github.com/tammoippen/plotille/pull/31#discussion_r401666597:

@@ -324,24 +363,39 @@ def height_vals(self):
     return self.Y

 def write(self, canvas, with_colors, in_fmt):
+

plot all points with optional text first, then lines

separating out points from lines allows single-point scatters to successfuly plot

make point iterators

Comment is wrong / old.

In requirements.txt https://github.com/tammoippen/plotille/pull/31#discussion_r401667261:

@@ -0,0 +1,5 @@

+numpy==1.15.3

Remove file. Dependency management is done with poetry https://python-poetry.org/

In plotille/_figure.py https://github.com/tammoippen/plotille/pull/31#discussion_r401671433:

plot all points with optional text first, then lines

separating out points from lines allows single-point scatters to successfuly plot

make point iterators

color = self.lc if with_colors else None

points = zip(map(in_fmt.convert, self.X), map(in_fmt.convert, self.Y))

for index, (x, y) in enumerate(points):

if(isinstance(self.text, list) and index <= len(self.text)):

text_for_point = self.text[index]

else:

text_for_point = self.text

canvas.point(x, y, color=color, overlay=self.overlay, marker=self.marker, text=text_for_point)

The text of Plot is used for every point? This should be the label for the legend, right?

In plotille/_figure.py https://github.com/tammoippen/plotille/pull/31#discussion_r401673571:
     # plot points
     for (x0, y0), (x, y) in zip(from_points, to_points):
canvas.point(x0, y0, color=color)
canvas.point(x, y, color=color)
     if self.interp == 'linear':

         canvas.line(x0, y0, x, y, color=color)
Can it be, that the line interpolations overwrite the text from above?

Also, if the plotting of points is moved to the code above, I would put all the code from 384ff into the if self.interp == 'linear': block.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/tammoippen/plotille/pull/31#pullrequestreview-385558949, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOYEKF6JSTSKDBJJKF4SHZ3RKNIDBANCNFSM4LJIW4GQ .

tammoippen commented 3 years ago

Closing as #38 implements most of the features.

tammoippen / plotille

Circleci project setup #31

Sorry for the late review. This is a initial review of the stuff that directly comes to my mind. I still want to play around with some of the features.

Please revert all changes in this file.

Remove file.

Let's have consistent examples and not use the if name == "main": pattern.

This way, somebody that wants to try the examples can just do a pip install -r requirements.txt and run the examples.

-- coding: utf-8 --

as the first line of the example.

same: no if name == "main"

can you run flake8 on the example folder as well and fix the issues?

why requests_cache and nor requests?

Instead of this file, can you create a README.md explaining how to run the examples?

No parentheses here and in all other if places, if not necessary.

classic:

Remove legacy comments.

classic:

ret = linesep.join(''.join(map(six.text_type, row)) for row in reversed(self._canvas))

return ret

combine _canvas (Dots) and overlay (text) so that overlay is on top of dots

This means, if the text in the overlay has a ' ', we might have dots between the text?

This is not a dependency of plotille and i would like to keep it that way. Please implement with stdlib funktions.

The test later can be something like: if marker:

plot all points with optional text first, then lines

separating out points from lines allows single-point scatters to successfuly plot

make point iterators

Comment is wrong / old.

Remove file. Dependency management is done with poetry https://python-poetry.org/

plot all points with optional text first, then lines

separating out points from lines allows single-point scatters to successfuly plot

make point iterators

The text of Plot is used for every point? This should be the label for the legend, right?