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

Height cannot be exactly configured #26

Open
neighthan opened this issue Jul 1, 2019 · 6 comments · May be fixed by #29
Open

Height cannot be exactly configured #26

neighthan opened this issue Jul 1, 2019 · 6 comments · May be fixed by #29
Assignees

Comments

@neighthan
Copy link
Contributor

Though there is a height parameter for the plotting function, this only roughly controls the height. It would be nicer to be able to exactly set the height. This is even more of an issue because the height isn't always off of the requested height by the same value, so it's not trivial to correct for this. Here are a couple of examples:

Requested height: 9; actual height: 10

$ pplot 1 10 --height 9
   10.00  ┤╭ 
    9.00  ┤│ 
    8.00  ┤│ 
    7.00  ┤│ 
    6.00  ┤│ 
    5.00  ┤│ 
    4.00  ┤│ 
    3.00  ┤│ 
    2.00  ┤│ 
    1.00  ┼╯ 

Requested height: 8; actual height: 10

$ pplot 1 10 --height 8
   10.00  ┼╭ 
    9.00  ┤│ 
    8.00  ┤│ 
    7.00  ┤│ 
    6.00  ┤│ 
    5.00  ┤│ 
    4.00  ┤│ 
    3.00  ┤│ 
    2.00  ┤╯ 
    1.00  ┼  

The issue is with the floor and ceil operators that are used in computing the actual number of rows.

Here's a quick fix for this in the Python version:

    target_height = cfg['height'] if 'height' in cfg else interval
    actual_height = float("inf")

    i = 0
    while actual_height > target_height:
        ratio = (target_height - i) / interval
        min2 = floor(minimum * ratio)
        max2 = ceil(maximum * ratio)
        actual_height = max2 - min2 + 1
        i += 1

    assert actual_height == target_height

I just keep decreasing the height value used by 1 until the actual height matches the target height. This has always given me an actual height that's exactly the target height in the quick tests I've done, but there's probably a better way to do this. I also considered decreasing max2 or increasing min2 by 1 directly, but then we'd also need to set a new value for ratio, at least to keep the current behavior, and I'm not sure what the appropriate way to do that would be.

@neighthan
Copy link
Contributor Author

Okay, the hacky fix mentioned above does not work in all cases; sometimes the actual_height will end up less than target_height. So, a more principled fix really is needed. (pplot 1 10 5 3 9 7 10 0.1 is an example command where that fix doesn't work).

@kroitor kroitor self-assigned this Jul 1, 2019
@kroitor
Copy link
Owner

kroitor commented Jul 1, 2019

@neighthan thx for your involvement in this! It might take some time to get to it, but I will add a fix asap!

@hartwork
Copy link

hartwork commented Nov 5, 2019

@kroitor any news on a fix?

@kroitor
Copy link
Owner

kroitor commented Nov 5, 2019

@hartwork this is addressed in the linked PR #29, hope to merge it soon.

@hartwork
Copy link

hartwork commented Nov 5, 2019

It looks like the last action and comment on #29 is from July. Would be great to get that fixed. Thanks!

@kroitor
Copy link
Owner

kroitor commented Nov 5, 2019

@hartwork it's been a bit of a delay due to my other work, but i'll do my best to resolve this in the near future.

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 a pull request may close this issue.

3 participants