HunterMcGushion / hyperparameter_hunter

Easy hyperparameter optimization and automatic result saving across machine learning algorithms and libraries

MIT License

706 stars 100 forks source link

Add informative comments #196

Open OverLordGoldDragon opened 5 years ago

coveralls commented 5 years ago

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
Change from base Build 831:	-7.2%
Covered Lines:	4331
Relevant Lines:	4955

💛 - Coveralls

OverLordGoldDragon commented 5 years ago

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.

HunterMcGushion commented 5 years ago

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!

codecov[bot] commented 5 years ago

Codecov Report

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

@@           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.

OverLordGoldDragon commented 5 years ago

@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 commented 5 years ago

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

HunterMcGushion commented 5 years ago

@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 commented 5 years ago

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!

OverLordGoldDragon commented 5 years ago

@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

HunterMcGushion commented 5 years ago

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