erdc / proteus

A computational methods and simulation toolkit

http://proteustoolkit.org

MIT License

88 stars 56 forks source link

adding numdiff lines to RANS2P.h and RANS2P2D.h, reverting rdls toler… #1216

Closed jhcollins closed 4 years ago

jhcollins commented 4 years ago

…ance back to 0.1*he in TwoPhaseFlow.py

Mandatory Checklist

Please ensure that the following criteria are met:

[ ] Title of pull request describes the changes/features
[ x] Request at least 2 reviewers
[ ] If new files are being added, the files are no larger than 100kB. Post the file sizes.
[ ] Code coverage did not decrease. If this is a bug fix, a test should cover that bug fix. If a new feature is added, a test should be made to cover that feature.
[ ] New features have appropriate documentation strings (readable by sphinx)
[x ] Contributor has read and agreed with CONTRIBUTING.md and has added themselves to CONTRIBUTORS.md

As a general rule of thumb, try to follow PEP8 guidelines.

Description

codecov[bot] commented 4 years ago

Codecov Report

Merging #1216 into master will decrease coverage by 0.00%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1216      +/-   ##
==========================================
- Coverage   52.09%   52.09%   -0.01%     
==========================================
  Files         526      526              
  Lines      108803   108803              
==========================================
- Hits        56684    56683       -1     
- Misses      52119    52120       +1

Impacted Files	Coverage Δ
proteus/TwoPhaseFlow/utils/Parameters.py	`89.81% <100.00%> (ø)`
proteus/tests/test_boundaryconditions.py	`97.46% <0.00%> (-0.13%)`	:arrow_down:

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 f338ae4...c557e5d. Read the comment docs.

zhang-alvin commented 4 years ago

Let me try duplicating the result on a docker container.

zhang-alvin commented 4 years ago

Just kidding, I don't think it's possible.

zhang-alvin commented 4 years ago

@cekees It looks like restarting the build is insufficient. Considering the small number of changes, I'm quite alright with simply skipping that test and having someone with osx actually take a closer look to understand the effects of the diff.

cekees commented 4 years ago

OK with me. @jhcollins do you want to just put in a line in .travis.yaml to exclude that test on osx? @ejtovar whenever you get a chance, could you please run that test and see if it fails on your machine?

zhang-alvin commented 4 years ago

Going to go ahead and merge this. @ejtovar feel free to add your observations in this chain even if the PR is closed.

jhcollins commented 4 years ago

Thanks alvin. I was holding off until eric could check it out, but im fine with merging 👍

Sent from my iPhone

On Jun 26, 2020, at 10:16 PM, Alvin Zhang notifications@github.com wrote:

Going to go ahead and merge this. @ejtovar feel free to add your observations in this chain even if the PR is closed.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.