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

Add LowerBound and UpperBound options #12

Merged
merged 2 commits into from Jun 23, 2023

Conversation

carlosms
Copy link
Contributor

@carlosms carlosms commented Apr 1, 2019

This PR adds a couple of options to set the min and max values of the Y axis.
I needed them for a personal project and figured it might be useful to other people.

Update: options name updated to LowerBound and UpperBound

@coveralls
Copy link

Pull Request Test Coverage Report for Build 42

  • 10 of 10 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.8%) to 79.921%

Totals Coverage Status
Change from base Build 47: 0.8%
Covered Lines: 203
Relevant Lines: 254

💛 - Coveralls

1 similar comment
@coveralls
Copy link

Pull Request Test Coverage Report for Build 42

  • 10 of 10 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.8%) to 79.921%

Totals Coverage Status
Change from base Build 47: 0.8%
Covered Lines: 203
Relevant Lines: 254

💛 - Coveralls

@coveralls
Copy link

coveralls commented Apr 1, 2019

Pull Request Test Coverage Report for Build 5349798357

  • 10 of 10 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 96.037%

Totals Coverage Status
Change from base Build 5302804267: 0.1%
Covered Lines: 315
Relevant Lines: 328

💛 - Coveralls

@jesseduffield
Copy link

I've been using this fork myself recently and can attest to its robustness. Let's get this baby merged!

@guptarohit
Copy link
Owner

Hi, @carlosms thanks for the PR :octocat: ❤️

These options are definitely needed, here I'm little skeptical about the name of the options. They actually mean MinLower, MinUpper; if possible, can we think of name other than Min & Max.

👋 @jesseduffield would like to have your views on this.

Naming is hard! Jeff Atwood's tweet 😄

@jesseduffield
Copy link

jesseduffield commented Jun 5, 2019

@guptarohit I'm not sure why MinLower and MinUpper are more correct. Do you mean MaxUpper?

What about LowerBound and UpperBound:

image

Although technically that's referring to data points on the graph.. damn.

This is tricky because as the end-user, the names which are most intuitive are coincidentally names that don't map well onto the actual terminology.

What about WindowMax and WindowMin?

@guptarohit
Copy link
Owner

guptarohit commented Jun 8, 2019

@jesseduffield if I understand correctly these options ensure the minimum lower bound of the graph (therefore MinLower) & minimum upper bound of the graph(therefore MinUpper),
as L64

It will be ignored if the series contains a bigger value.

Whereas, MaxUpper or Max appear like it would set the max possible point on the graph. Please correct me if I'm wrong.

@jesseduffield
Copy link

@guptarohit I'm not quite sure what you mean. Do you want to use MinLower and MaxUpper or MinLower and MinUpper?

These comments don't seem to be cohesive:

They actually mean MinLower, MinUpper
maximum upper bound of the graph(therefore MaxUpper)
MaxUpper or Max appear like it would set the max possible point on the graph.

At any rate, I'm find with MinLower and MaxUpper

@guptarohit
Copy link
Owner

hi @jesseduffield, I actually meant MinLower and MinUpper (edited my comment now, please check)

@jesseduffield
Copy link

@guptarohit I am still a little confused. Lets say that we're using this PR's terminology, and I set Min to 0 and Max to 10, but then my data set ranges from -10 to 20. The lowest value on the graph will be 0 and the highest value will be 10. I'm not sure why the Max would be better named MinUpper in that case

@guptarohit
Copy link
Owner

@jesseduffield I'm afraid it won't be like that as per this PR, please check the test case #16 or check the O/P by following example:

data := []float64{-2, 1, 11, 2, -10, 5, 7, 11, -9, -4, -1, 5, 13, -2}
asciigraph.Plot(data, asciigraph.Min(0), asciigraph.Max(10))
  15.00 ┤       ╭╮     
  14.00 ┤       ││     
  13.00 ┤       ││  ╭╮ 
  12.00 ┤       ││  ││ 
  11.00 ┤ ╭╮   ╭╯│  ││ 
  10.00 ┤ ││   │ │  ││ 
   9.00 ┤ ││   │ │  ││ 
   8.00 ┤ ││   │ │  ││ 
   7.00 ┤ ││  ╭╯ │  ││ 
   6.00 ┤ ││  │  │  ││ 
   5.00 ┼ ││ ╭╯  │ ╭╯│ 
   4.00 ┤ ││ │   │ │ │ 
   3.00 ┤ ││ │   │ │ │ 
   2.00 ┤ │╰╮│   │ │ │ 
   1.00 ┤╭╯ ││   │ │ │ 
   0.00 ┤│  ││   │ │ │ 
  -1.00 ┤│  ││   │╭╯ │ 
  -2.00 ┼╯  ││   ││  ╰ 
  -3.00 ┤   ││   ││    
  -4.00 ┤   ││   ││    
  -5.00 ┤   ││   ││    
  -6.00 ┤   ││   ││    
  -7.00 ┤   ││   ││    
  -8.00 ┤   ││   ││    
  -9.00 ┤   ││   ╰╯    
 -10.00 ┤   ╰╯         

@jesseduffield
Copy link

That's strange, I'm not really sure whether Min or Max are having any effect at all there. I would have expected it to have a max y-axis value of 10, and a min of 0. @carlosms is the above example an expected output?

@medzernik
Copy link

Any progress on this?

@guptarohit guptarohit force-pushed the master branch 6 times, most recently from 228c545 to 5deec54 Compare May 2, 2022 19:25
@guptarohit guptarohit force-pushed the min-max branch 2 times, most recently from f0b23df to 9f3ce84 Compare June 21, 2023 21:54
@guptarohit guptarohit changed the title Add Min and Max options Add LowerBound and UpperBound options Jun 21, 2023
@guptarohit
Copy link
Owner

Updated options names to LowerBound and UpperBound. This PR also resolves #26

@guptarohit guptarohit added the enhancement New feature or request label Jun 22, 2023
@guptarohit guptarohit linked an issue Jun 22, 2023 that may be closed by this pull request
@guptarohit guptarohit merged commit a95f7a0 into guptarohit:master Jun 23, 2023
8 checks passed
@guptarohit guptarohit mentioned this pull request Apr 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add option to set fixed range for values
5 participants