Fix for exact specification of plot height. #29
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This removes a handful of unnecessary uses of `abs`, `float`, and `int` and also puts in a check to ensure that the maximum value for the plot is greater than the minimum value. No functionality is changed here.
This changes a bit of the logic for making the plot, some of which wasn't really necessary but made it easier / less confusing for me to work on it. `ratio` is the reciprocal of what it was before (it's now the change in y when you move up one row in the plot). The plot is stored in reverse order compared to before when in list form (e.g. `result[0]` is now the bottom row of the plot instead of the top one; I reverse the rows before joining them into the final plot string to make up for this). The actual change is a rework to how we convert from the data space to the row space of the plot which makes it (IMHO) less complex (removes `max2` and `min2`) and easy to guarantee exactly what the height is. Admittedly, I'm not sure what the thought process was behind the old method, so there may be some drawback that I haven't considered.
|
This wasn't exactly intended to fix #25, but I believe it does (certainly in my tests). |
|
This should give a merge conflict if #27 is merged first; doing |
|
@neighthan hey ) Pardon for a long wait, didn't really have the time to dive into it thoughtfully (was mostly occupied with ccxt), but hope to get to it asap! Thx again for your involvement! Appreciate it very much! |
This was referenced Nov 5, 2019
|
Any updates on this? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #26, fixes #25.
This changes a bit of the logic for making the plot, some of which wasn't really necessary but made it easier / less confusing for me to work on it.
ratiois the reciprocal of what it was before (it's now the change in y when you move up one row in the plot). The plot is stored in reverse order compared to before when in list form (e.g.result[0]is now the bottom row of the plot instead of the top one; I reverse the rows before joining them into the final plot string to make up for this). The actual change is a rework to how we convert from the data space to the row space of the plot which makes it (IMHO) less complex (removesmax2andmin2) and easy to guarantee exactly what the height is.Caveats
max2andmin2were needed before instead of working withmaximumandminimumdirectly, as I do in this version. Is there some drawback to the method I use here that I'm missing?(#28 should be merged before this one, which starts from the cleaned up Python code; I was working on a few different things in parallel, but it got to be a pain to have all the PRs completely independent. Just look at the second commit to see the changes that this PR introduces).