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

Add informative comments #196

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

Add informative comments #196

wants to merge 2 commits into from

Conversation

OverLordGoldDragon
Copy link

No description provided.

@coveralls
Copy link

coveralls commented Sep 17, 2019

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

💛 - Coveralls

@OverLordGoldDragon
Copy link
Author

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
Copy link
Owner

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
Copy link

codecov bot commented Sep 17, 2019

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.

@OverLordGoldDragon
Copy link
Author

OverLordGoldDragon commented Sep 17, 2019

@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
Copy link
Author

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
Copy link
Owner

@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
Copy link
Owner

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
Copy link
Author

OverLordGoldDragon commented Sep 24, 2019

@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
Copy link
Owner

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

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.

None yet

3 participants