mutationpp / Mutationpp

The MUlticomponent Thermodynamic And Transport library for IONized gases in C++
GNU Lesser General Public License v3.0
106 stars 58 forks source link

Closing #114 mppshock #119

Closed grgbellasvki closed 3 years ago

grgbellasvki commented 4 years ago

Merge request in response to issue #114. Submitted in order to have the code reviewed. The code is working, but testing still needed.

codecov[bot] commented 4 years ago

Codecov Report

Merging #119 (584f919) into master (d49d9c0) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #119   +/-   ##
=======================================
  Coverage   70.21%   70.21%           
=======================================
  Files         134      134           
  Lines        8458     8458           
=======================================
  Hits         5939     5939           
  Misses       2519     2519           

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 d49d9c0...584f919. Read the comment docs.

rdbisme commented 4 years ago

I can comment on the cmake stuff, and it looks ok to me. Thanks for fixing the leftover white spaces.

PS: I think you need to rebase this branch over the last commit on master

jbscoggi commented 3 years ago

Hey @grgbellas, I think this PR might have gotten lost in the wind a little but it seems nearly ready to merge. Could you take care of removing using namespace std? I think that's the only outstanding issue. It would be great to have this capability finally shipped with M++!

grgbellasvki commented 3 years ago

Sounds good @jbscoggi. Let's spend 5 minutes during the next M++ meeting to discuss about unit testing of the new feature. As you know, we have implemented shocking so many times outside of Mutation to have a certain level of confidence on the results, but a simple comparison with CEA would be great.

I think the latest push doesn't have using namespace std; anymore. Can you verify that this is the case. Honestly, looking at the code 1 year after it was implemented, I would modify/modernize a few parts of mppshock.cpp, but this is of lower priority. It would be very interested in a "strict" code review from your side, if you have the time/patience.

jbscoggi commented 3 years ago

Hey @grgbellas yes it seems the latest push is good to go. I will go ahead and merge. We can discuss the validation testing during the meeting. Would you mind to prepare a very small demo? Maybe also a single slide with the equations/assumptions behind the code?