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

Implemented graph value clamping #2000

Merged
merged 3 commits into from
Jul 30, 2024
Merged

Implemented graph value clamping #2000

merged 3 commits into from
Jul 30, 2024

Conversation

donutAnees
Copy link
Contributor

Checklist

  • I have described the changes
  • I have linked to any relevant GitHub issues, if applicable
  • Documentation in doc/ has been updated
  • All new code is licensed under GPLv3

Description

Before

Values that fall below the threshold of visibility in a given graph are not seen, leading to small values being omitted.

After

A new flag, '-m value', has been implemented, allowing the user to specify the threshold value between 0 and 5. This will enable values below the threshold to be displayed.

downspeedgraph en0 50,280 ADFF2F 32CD32 -t -x -m 5
Screenshot 2024-07-27 at 1 03 35 AM

Fixes #1060

Copy link

netlify bot commented Jul 26, 2024

Deploy Preview for conkyweb ready!

Name Link
🔨 Latest commit 1db645b
🔍 Latest deploy log https://app.netlify.com/sites/conkyweb/deploys/66a85d0dff5f210008caa862
😎 Deploy Preview https://deploy-preview-2000--conkyweb.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@github-actions github-actions bot added documentation Issue or PR that suggests documentation improvements sources PR modifies project sources rendering Issue or PR related to rendering text Issue or PR related to `conky.text` variables labels Jul 26, 2024
@brndnmtthws
Copy link
Owner

Thanks for the PR! My only question is, are we sure "clamping" is the right term for this? When I think of clamping, I tend to think about having an upper bound limit or lower bound (i.e., setting a minimum and maximum possible range of values). This is a slightly different: we're essentially setting a lower bound limit, but only for nonzero values. It's different from having a floor or ceiling, as again–those apply whether you have a zero or not. I wonder if there is a better term to describe it?

Clamp is basically min(max(x, min_val), max_val). But what we're doing here is more like if x != 0 then max(min_val, x) else x end.

Maybe even just calling this "nonzero minimum" or something is more descriptive.

Thoughts?

@brndnmtthws brndnmtthws added the feature New feature PR or issue label Jul 26, 2024
@donutAnees
Copy link
Contributor Author

donutAnees commented Jul 27, 2024

Hey! Those are very valid points, traditional definition of clamping would not be very applicable here. The reason I thought clamping made sense is that we are setting a lower bound with a condition, so it's more like 'conditional clamping' rather than the default definition. However, I believe the "nonzero minimum" would be a good term to describe this or something like "nonzero lowerbound" would be better. I’m really interested to hear your thoughts on this!

29 July 2024
@brndnmtthws Hey just a follow up on this!

@brndnmtthws
Copy link
Owner

brndnmtthws commented Jul 29, 2024

Hey! Those are very valid points, traditional definition of clamping would not be very applicable here. The reason I thought clamping made sense is that we are setting a lower bound with a condition, so it's more like 'conditional clamping' rather than the default definition. However, I believe the "nonzero minimum" would be a good term to describe this or something like "nonzero lowerbound" would be better. I’m really interested to hear your thoughts on this!

I think these are all good suggestions: conditional clamping, or nonzero minimum/lowerbound make sense. I think if you update the docs to use that language (I defer to you on which to use, as the PR author), then this PR should be good to go!

@donutAnees
Copy link
Contributor Author

donutAnees commented Jul 30, 2024

Hey! Those are very valid points, traditional definition of clamping would not be very applicable here. The reason I thought clamping made sense is that we are setting a lower bound with a condition, so it's more like 'conditional clamping' rather than the default definition. However, I believe the "nonzero minimum" would be a good term to describe this or something like "nonzero lowerbound" would be better. I’m really interested to hear your thoughts on this!

I think these are all good suggestions: conditional clamping, or nonzero minimum/lowerbound make sense. I think if you update the docs to use that language (I defer to you on which to use, as the PR author), then this PR should be good to go!

Thanks a lot for the freedom! I've made the changes to the doc. Cheers!

@brndnmtthws brndnmtthws merged commit e03891f into brndnmtthws:main Jul 30, 2024
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Issue or PR that suggests documentation improvements feature New feature PR or issue rendering Issue or PR related to rendering sources PR modifies project sources text Issue or PR related to `conky.text` variables
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Enhancement]: clamp (ceil) graph values to first unit
2 participants