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

Cpu load #36

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Cpu load #36

wants to merge 4 commits into from

Conversation

dherre3
Copy link

@dherre3 dherre3 commented Jul 2, 2017

Two things done in this pull request.
Added cpu support to run, config.
Added missing semicolons

Copy link
Member

@elavoie elavoie left a comment

Choose a reason for hiding this comment

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

I am actually using the Standard programming style for JavaScript. There are no semi-colons on purpose:

https://github.com/standard/standard

Could you remove the semi-colons you added?

Copy link
Member

@elavoie elavoie left a comment

Choose a reason for hiding this comment

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

Please check your modifications against the JavaScript Standard format:

https://github.com/standard/standard

Could you also add a page in the wu-wei-handbook (https://github.com/Sable/wu-wei-handbook) that describes the technique:

  1. Motivation and Use Case
  2. Computation Method of CPU Load
  3. Correlation Results
    -> on a few benchmarks that justifies introducing the measurements and the effectiveness of the method in detecting CPU Load

I will integrate the documentation (or a summary of it) in the ongoing paper and you can revise it after.

Then add a summary and a link to the wu-wei-handbook over the 'cpuAverage' method for later reference.

Moreover, I thought the approach would be transparent and would therefore not require extending the information provided to the user during a report. I think it will be confusing because as a user I would expect it to refer to the CPU Load during the experiment. Could you remove it?

I think it would be better to decide on a CPU load level above which a warning is generated during the run phase if crossed. The warning can be written on the standard error with an error message like: "WARNING: The average background CPU load is X% and may affect the validity of results obtained for run /path/to/run/results. To discard the results, delete the directory." As long as the level is not reached it would be completely transparent to the experimenter. It would therefore act as a safeguard: no increased cognitive load on users that know what they are doing but warnings for those that don't :-).

@dherre3
Copy link
Author

dherre3 commented Jul 3, 2017

I really think you need to double check your sources in terms of "standards", the truth is that repository is as much of a standard as my own arbitrary rules on how to write Javascript. There is not such a thing as a real and unique Javascript style standard. That repository has a very misleading name which is why you might have been drawn to use them. You can see part of the discussion on this issue, standard/standard#78. Or here: https://www.reddit.com/r/reactjs/comments/4eexcy/semicolons_or_not/. There also many other Javascript stylers standards that use semicolons. Basically, it comes down to personal preference, although most of the Javascript community leans towards the use of semicolons. Big companies such as Google, Airbnb, ReactJs, Typescript, use mandatory semicolons in their own standards (See reddit post). For our purposes, I made the mistake of touching your code. I should have respected your style. From now on, I will not touch your code to change styles and since you have written your code with that standard, I will use that for consistency in the project. I, however, want to make clear that they misleadingly use the "standard" name in that repository.

@elavoie
Copy link
Member

elavoie commented Jul 3, 2017

@dherre3 I was well aware that it was one guy standard shared with other influential developers and not a community standard. I also agree the choice of name is misleading. I tried it and it worked well for me so I used it. As you correctly said, I was asking you to change it for the sake of consistency in the repository. If I ever submit PR to your projects and they end up using semi-colons, I will gladly keep them :-).

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 this pull request may close these issues.

2 participants