Skip to content
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

[canvas-] draw lines at user-defined x or y #2489

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

midichef
Copy link
Contributor

@midichef midichef commented Aug 6, 2024

This is a draft of a feature to draw vertical or horizontal lines on a Canvas.

For now I used the command names: add-line-x, remove-line-x, clear-lines-x. Any better names I could use instead?

When finalized, this PR closes #2482.

visidata/canvas.py Outdated Show resolved Hide resolved
@@ -808,6 +865,13 @@ def deleteSourceRows(self, rows):
Canvas.addCommand('g'+ENTER, 'dive-visible', 'vs=copy(source); vs.rows=list(rowsWithin(plotterVisibleBox)); vd.push(vs)', 'open sheet of source rows visible on screen')
Canvas.addCommand('gd', 'delete-visible', 'deleteSourceRows(rowsWithin(plotterVisibleBox))', 'delete rows on source sheet visible on screen')

Canvas.addCommand('', 'add-line-x', 'sheet.gridlines_x += [float(x) for x in input("add line(s) at x = ", value="0.0", defaultLast=True).split()]; sheet.refresh()', 'draw a vertical line at x-values (space-separated)')
Canvas.addCommand('', 'add-line-y', 'sheet.gridlines_y += [float(y) for y in input("add line(s) at y = ", value="0.0", defaultLast=True).split()]; sheet.refresh()', 'draw a horizontal line at y-values (space-separated)')
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should use col.type() (dates are a numeric type). Ideally the default value should be something reasonable, maybe the median of the data range? And let's use type="gridlinex"/gridliney with both the add and remove, so people can scroll up to remove the most recently added gridline.

For the longnames, let's not use the 'add/remove/clear' verbs since current 'add-' commands are used to modify the data. What can we use to indicate this is an interface-only command?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of 'add/remove/clear', how about 'draw/erase/erase'?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That works for me! draw- especially is very clearly interface only.

Copy link
Owner

@saulpw saulpw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this, @midichef! I think it looks nice. Maybe we could add the theme elements to the ascii themes with | and -?

@midichef
Copy link
Contributor Author

Okay, the last commit addresses all the feedback on this feature so far.

One choice that I'd like feedback on is the parsing of input for gridline values. It currently allows multiple values as a space-separated string. E.g. draw-line-x 3.0 5.0 7.0. But that means input can't take strings that are naturally space separated, like dates such as Jan 1, 1935. So right now, input when drawing/erasing gridlines requires date strings that contain no spaces. Dates must be input in the form YYYY-MM-DD regardless of the setting of disp_date_fmt. Similarly, the input strings can't contain commas, because we use commas to separate x-values in the case when the graph uses multiple x-columns.

An alternative design would be to accept input in any form, but restrict the inputs for draw-line-* and erase-line-* to only take one value at a time. Let me know if that's better.

Another change in this commit, I moved the gridlines out of Canvas and into GraphSheet, because they depend on things like xcols that don't exist for Canvas. Grid lines are more naturally part of GraphSheet than for Canvas. Just like axis labels are part of GraphSheet and not Canvas.

@midichef midichef marked this pull request as ready for review August 20, 2024 01:02
@cool-RR
Copy link
Contributor

cool-RR commented Aug 20, 2024

Hey,

Thanks for implementing this.

  1. Looks good overall, including x lines.
  2. I do think the term "refline" would be better than "line".
  3. Is there a way to change the color of the lines?
  4. Check this out:

I drew a line at 0.4 (I think):

screenshot_HOPPER_2024-08-20_103604

But when I zoom out I don't see it:

screenshot_HOPPER_2024-08-20_103612

Maybe it's because there are too many data points around that Y value. However, notice how there's clear space both on the left and on the right of the data. Could you extend the lines to there? As long as they don't clash with the tick numbers, of course.

@midichef
Copy link
Contributor Author

midichef commented Aug 20, 2024

To change the color of the line you can add a line to your .visidatarc like options.color_gridline = 'green'
We will probably change the name to something more consistent with other color names like color_graph_refline.

I'm not sure why the y=0.4 line disappeared on zooming in your images. That looks like a bug to me. The refline should be drawn on the left and right of the data.

I can't reproduce it in my limited testing. Can you find a way to consistently reproduce that, and post a sample data file, command log, and terminal dimensions in characters (such as 120x40)?

@midichef midichef marked this pull request as draft August 21, 2024 07:06
@cool-RR
Copy link
Contributor

cool-RR commented Aug 21, 2024

@midichef

Data file and command log: https://www.dropbox.com/scl/fo/5vpb430tpnyq9iinbcljk/AKPM09_NkdomvwodvrpyFCE?rlkey=ssbhjdqs8w92dblxzkhdmwne1&dl=0

Terminal size:

➜  Desktop echo "Rows: $(tput lines), Columns: $(tput cols)"
Rows: 33, Columns: 133

One thing that may not be clear in that command log is that the selected rows (i.e. columns) are reward.a0.coeval to reward.a3.coeval, a total of 4 rows/columns.

@midichef
Copy link
Contributor Author

Thanks for the data files. They reproduced the missing refline on my system.

The problem was that certain kinds of zoom, like the ones used by set-y and set-x, were not recalculating the view window for the next screen redraw. I've fixed that behavior.

I've also changed the command names to draw-line-* to draw-refline-*, etc.

I believe that addresses all the feedback so far. Any other issues?

@cool-RR
Copy link
Contributor

cool-RR commented Aug 21, 2024

Great, thank you. Yes, this addresses all my feedback. I'll probably start using this feature when the next release of visidata is made. Thanks for implementing this.

@midichef midichef marked this pull request as ready for review August 23, 2024 01:49
@midichef midichef marked this pull request as draft August 24, 2024 23:29
@midichef midichef marked this pull request as ready for review August 24, 2024 23:59
@midichef
Copy link
Contributor Author

midichef commented Aug 26, 2024

@cool-RR
I noticed visidata makes a small and subtle error when setting the y range via y (resize-y-input).

After I do y 1 5, the actual range is 1.02649 to 5.02649, not 1.0 to 5.0. So just be aware that in visidata v3.0 versions, if you set the y axes, the bottom edge isn't quite what it seems. Points that you'd expect to see at the bottom edge of the graph, like y=1.0, would not be shown.

The error has existed since my commit abc9aff from June 2023, and I've submitted a fix here that will be in the next release of visidata.

Please let me know if you ever notice any similar errors in the edges of ranges/boxes when using graphs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[] Feature request: Horizontal guide lines in plots
3 participants