search

UT-CHG / BET

Python package for data-consistent stochastic inverse and forward problems.
http://ut-chg.github.io/BET
Other
11 stars 21 forks source link

Linting (+minor bug fix) #311

Closed mathematicalmichael closed 5 years ago

mathematicalmichael commented 5 years ago

Re: #306

codecov[bot] commented 5 years ago

Codecov Report

Merging #311 into master will increase coverage by 0.03%. The diff coverage is 81.54%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #311      +/-   ##
=========================================
+ Coverage   78.06%   78.1%   +0.03%     
=========================================
  Files          22      22              
  Lines        3889    3895       +6     
=========================================
+ Hits         3036    3042       +6     
  Misses        853     853
Impacted Files Coverage Δ
bet/sampling/LpGeneralizedSamples.py 94.73% <ø> (ø) :arrow_up:
bet/sample.py 80.15% <ø> (ø) :arrow_up:
bet/calculateP/calculateP.py 88.72% <100%> (ø) :arrow_up:
bet/calculateP/indicatorFunctions.py 100% <100%> (ø) :arrow_up:
bet/postProcess/plotDomains.py 60% <100%> (ø) :arrow_up:
bet/calculateP/simpleFunP.py 70.95% <100%> (+0.09%) :arrow_up:
bet/postProcess/postTools.py 52.38% <50%> (ø) :arrow_up:
bet/postProcess/plotP.py 63.07% <60.71%> (+0.14%) :arrow_up:
bet/sensitivity/gradients.py 93.83% <66.66%> (ø) :arrow_up:
bet/sampling/adaptiveSampling.py 84.24% <75.6%> (+0.15%) :arrow_up:
... and 4 more

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 10f6e8d...f24a6ce. Read the comment docs.

smattis commented 5 years ago

@mathematicalmichael I don't completely love this, because I think that much of the stuff that it is changing was already following PEP 8 guidelines. Surely the result does also follow PEP 8, but the changes are not necessary. We also had previously excluded the tests from linting for simplicity, but there is no reason they can't be updated.

Also, @lcgraham and I had a policy that linting pull requests would have no other changes to the code in linting pull requests, so that they don't have to be reviewed line-by-line which can be very tedious.

smattis commented 5 years ago

@mathematicalmichael Is this https://github.com/UT-CHG/BET/pull/311/commits/f24a6ce3f5fd9b91f09ed093b3e11c7f0a493d68 the only non-linting change?

mathematicalmichael commented 5 years ago

@mathematicalmichael Is this f24a6ce the only non-linting change?

yes. some travis builds failed because of this, and passed when re-run. one of the times I managed to catch it failing and track down the line number. So.. from my perspective, you weren't going to merge something that was failing a check.

Sorry, I didn't know about the policy, I only discovered it when the Travis build failed! (the nosetests were still passing for me locally). Would the correct flow have been to open a new PR from master with JUST the bug fix, wait for that to be accepted? Would that have triggered Travis to re-build this PR and see if it would work?

Would I have to merge the "fixed" master into this branch with a new commit to trigger a new build # from Travis? then that linting commit + one merge containing the bug fix from master gets merged into master?

mathematicalmichael commented 5 years ago

@mathematicalmichael I don't completely love this, because I think that much of the stuff that it is changing was already following PEP 8 guidelines. Surely the result does also follow PEP 8, but the changes are not necessary. We also had previously excluded the tests from linting for simplicity, but there is no reason they can't be updated.

Also, @lcgraham and I had a policy that linting pull requests would have no other changes to the code in linting pull requests, so that they don't have to be reviewed line-by-line which can be very tedious.

my thinking was that I kept finding places with missing spaces and wanted to fix them, but got tired of tracking them one-by-one. Lindley suggested autopep so I decided to do a pass with it. I frankly, love the changes. I think they are much more visually pleasing, and all the things I wanted to fix (inconsistency with Test and test, one-space/no-space/two-space, etc.) were all handled, but I know more than that was also done.

smattis commented 5 years ago

@mathematicalmichael I agree that everything does look good after the linting. Just keep in mind for the future, that if you do automated linting, please do it before a new pull request, so that your submitted code is already following PEP 8, or do a separate pull request which only does linting. This makes things easy to review. It would be bad for something to get lost in the shuffle in a 2,000 line change commit.