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.

Sign up for GitHub

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

Jump to bottom

Add a flag for fan speed #63

Merged
merged 3 commits into from
May 4, 2019
Merged

Add a flag for fan speed #63

merged 3 commits into from
May 4, 2019

Conversation

bethune-bryant
Copy link
Contributor

Fixes #62

I used what was done in #42 , but added the command line flag and unit test.

@bethune-bryant
Copy link
Contributor Author

@wookayin Does this look good to you?

@wookayin
Copy link
Owner

wookayin commented May 2, 2019

@bethune-bryant Thank for your contribution. Can you fix the travis CI failure?

@wookayin wookayin self-assigned this May 2, 2019
wookayin
wookayin requested changes May 2, 2019
View reviewed changes
Copy link
Owner

@wookayin wookayin left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Generally looks good to me, but we several things could be improved. Plus, Travis CI is failing

gpustat/core.py Outdated Show resolved Hide resolved
gpustat/core.py
@@ -189,8 +199,12 @@ def _repr(v, none_value='??'):
# temperature and utilization
reps = "%(C1)s[{entry[index]}]%(C0)s " \
"%(CName)s{entry[name]:{gpuname_width}}%(C0)s |" \
"%(CTemp)s{entry[temperature.gpu]:>3}'C%(C0)s, " \
"%(CUtil)s{entry[utilization.gpu]:>3} %%%(C0)s"
"%(CTemp)s{entry[temperature.gpu]:>3}'C%(C0)s, "
Copy link
Owner

Choose a reason for hiding this comment

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

We should use a dedicated color for fan speed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think yellow/bold_yellow is okay?

@wookayin wookayin changed the title Add a flag for fan speed. Add a flag for fan speed May 4, 2019
@wookayin wookayin merged commit e81414a into wookayin:master May 4, 2019
@wookayin
Copy link
Owner

wookayin commented May 4, 2019

Looks good to me, thanks for your contribution. I'll follow up with a bit of more modification.

wookayin added a commit that referenced this pull request May 4, 2019
@wookayin
Use 'fan' instead of 'fan_speed' (#63)
25ebcd4
@wookayin
Copy link
Owner

wookayin commented May 4, 2019

I have changed fan -> fan_speed to clarify its meaning. Regarding color, I feel using different color (e.g. cyan) would be better than using yellow (somehow conflicts with memory). Any thoughts?

@bethune-bryant
Copy link
Contributor Author

Regarding color, I feel using different color (e.g. cyan) would be better than using yellow (somehow conflicts with memory). Any thoughts?

It looked like all of the colors were used, so I just chose yellow. Cyan would be good, because it's only used for the gpu id. There are also bright versions for each color that could be used.

wookayin added a commit that referenced this pull request Jul 22, 2019
@wookayin
Use cyan color for fan speed (#63)
8f9cd8e
@wookayin wookayin mentioned this pull request Jul 22, 2019
Closed
@wookayin wookayin added this to the 0.6 milestone Jul 22, 2019
@bethune-bryant bethune-bryant deleted the add_fan_percentage branch July 24, 2019 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wookayin wookayin wookayin requested changes

Assignees

@wookayin wookayin

Labels
new feature
Projects
None yet
Milestone
0.6
Development

Successfully merging this pull request may close these issues.

Show fan speed
2 participants
@bethune-bryant @wookayin