hyperparameter_hunter icon indicating copy to clipboard operation
hyperparameter_hunter copied to clipboard

Add informative comments

Open OverLordGoldDragon opened this issue 5 years ago • 10 comments

OverLordGoldDragon avatar Sep 17 '19 00:09 OverLordGoldDragon

Pull Request Test Coverage Report for Build 833

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 356 unchanged lines in 17 files lost coverage.
  • Overall coverage decreased (-7.2%) to 87.407%

Files with Coverage Reduction New Missed Lines %
hyperparameter_hunter/init.py 1 85.29%
hyperparameter_hunter/i_o/reporting.py 1 86.64%
hyperparameter_hunter/sentinels.py 1 95.65%
hyperparameter_hunter/settings.py 1 90.32%
hyperparameter_hunter/space/space_core.py 1 94.81%
hyperparameter_hunter/utils/general_utils.py 1 99.13%
hyperparameter_hunter/utils/optimization_utils.py 1 93.65%
hyperparameter_hunter/experiments.py 2 92.7%
hyperparameter_hunter/keys/makers.py 13 89.29%
hyperparameter_hunter/optimization/protocol_core.py 21 91.0%
<!-- Total: 356
Totals Coverage Status
Change from base Build 831: -7.2%
Covered Lines: 4331
Relevant Lines: 4955

💛 - Coveralls

coveralls avatar Sep 17 '19 00:09 coveralls

Any clue what these 'checks' are complaining about? All I did was newline some code and add comments - and coverage dropped 5%. I'm considering getting these for my own repos, but if this behavior isn't configurable, that's a big minus.

OverLordGoldDragon avatar Sep 17 '19 19:09 OverLordGoldDragon

Sorry for not getting to this sooner, and about the weird coverage issue. I've noticed that Coveralls can sometimes be finicky about combining coverage files from multiple Travis jobs. Clearly this PR isn't dropping any coverage, so we can ignore that.

The Travis check is complaining about Black formatting, which I do want to adhere to. If you've installed Black, you can follow the (admittedly lacking) CONTRIBUTING "Getting Started" section to fix the formatting. If you'd rather not install Black, the Travis job log does specify which lines should be reformatted and how.

Once we're all set on the formatting, reviewing will be easier, and we can merge this. I think the biggest Black formatting issues are probably that it wants two spaces before an inline comment, and it doesn't like the two consecutive octothorpes.

Thanks for your work! This is a great improvement!

HunterMcGushion avatar Sep 17 '19 21:09 HunterMcGushion

Codecov Report

Merging #196 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #196   +/-   ##
=======================================
  Coverage   94.61%   94.61%           
=======================================
  Files          46       46           
  Lines        4955     4955           
=======================================
  Hits         4688     4688           
  Misses        267      267

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 41c1905...cdba216. Read the comment docs.

codecov[bot] avatar Sep 17 '19 22:09 codecov[bot]

@HunterMcGushion Consistency on cost of customizability - fair enough; glad your config supports additional whitespaces for vertical alignment purposes - other repos yelled about it.

Also, this PR page gives me a sensation of walking into a robo factory where you're approached by autonomous assistants upon attempting a change to the repo. (Not necessarily bad - in fact, rather unique for me)

OverLordGoldDragon avatar Sep 17 '19 22:09 OverLordGoldDragon

I seem to have broke it even more by editing as suggested - I'll let you take it from here; don't have to merge my PR, it was intended as a demo anyway. Thanks for the response

OverLordGoldDragon avatar Sep 17 '19 22:09 OverLordGoldDragon

@OverLordGoldDragon, yeah, I'm a big fan of keeping the codebase consistent. Unfortunately, in this case, it's led to some headaches, and I apologize for that.

sensation of walking into a robo factory where you're approached by autonomous assistants

I certainly understand the feeling! Despite their odd behavior from time to time (now), I find that the robots are worth the trouble and usually end up saving me from shooting myself in the foot.

I'll let you take it from here

Understood! Thanks for taking the time to make the PR and give me some feedback. I'm sorry again for the strange behavior of the robot assistants. I'll try to cherry-pick your commit or something to ensure your name stays attached to the work!

HunterMcGushion avatar Sep 18 '19 23:09 HunterMcGushion

Looks like the reason everything was breaking is because a new Keras version was released (2.3.0), which Travis was using automatically. Our previous, passing builds were using 2.2.5. I'm looking into specifically what caused the problems, but I just thought you might be curious!

HunterMcGushion avatar Sep 24 '19 20:09 HunterMcGushion

@HunterMcGushion Thanks for notifying - a bit off-topic, but I'm configuring my own Travis, and would like to ignore multiple error codes; barely know anything about Linux, so using

script:
  - ./test.sh --ignore=E127, E128, E221, E251

fails. Any suggestion or tutorial link as to how to accomplish this? Couldn't find much in Travis docs. Thanks ahead

OverLordGoldDragon avatar Sep 24 '19 21:09 OverLordGoldDragon

Yeah Travis was pretty hard for me to get started, too. Since this isn't related to the HH issue, are you available on Slack? I'd be happy to try to help you via Direct Message. You can find me on the HH Slack channel

HunterMcGushion avatar Sep 24 '19 21:09 HunterMcGushion