Apply code review changes to MCNP files #665

kkiesling · 2020-03-27T21:13:51Z

Applying the changes from the professional code review (#660) to the MCNP related files. The changes are as follows:

cleaning up commented lines in cmake files (original comment)
changes error messages to std::cerr instead of std::cout (original comment)
move strings for Graveyard and Vacuum into variables (original comment)
uses C++11 standards for converting int/doubles to strings (original comment)

The one comment that I did NOT address was this comment on the readability of using #ifdefs in function calls (see here for more information on why). It's doable, just will be done in a separate PR since it will likely be a larger more significant change.

pshriwise

These look like positive changes to me @kkiesling. Thanks for getting us started on incorporating the PullRequest changes.

pshriwise · 2020-03-27T23:26:13Z

src/mcnp/mcnp_funcs.cpp

        imp = 1.0;
        // otherwise as the map says
      } else {
        imp = DMD->importance_map[entity][particle_name];
      }
-      importances += "imp:" + mcnp_name + "=" + _to_string(imp) + " ";
+      importances += "imp:" + mcnp_name + "=" + std::to_string(imp) + " ";


It seems that std::to_string is always going to supply 6 decimal places for floating-point types, even if they aren't needed. The test output of dagmcnp_unit_tests expects the lcad output to have integer values for the neutron/photon importances and it's causing CI to fail.

We can either change the expected test output or we could get these values as integers before writing them to the lcad file. Given that MCNP treats these importances as floating-point values, I'd say we should update the expected results in our test suite. I'll defer to you and @makeclean on this though as you're both far more knowledgable than I :)

mcnp_funcs.cpp actually defines the importances as doubles. We could define it as an int . If the input to to_string() is an int, then there's no trailing zeros. But this is probably not recommended because users can define an importance as a doubles.

I am going to update the tests to expect doubles there.

Didn't mean that mcnp_funcs.cpp defines the importance as an integer.. just that the expected test output has something that looks like an int. :) In the end, the important part is how MCNP interprets them when determining how we should write them IMO, so I back your call to update the expected test output to expect doubles.

Right- Sorry that was my own processing of what was wrong because I was confused by the trailing zeros (I assumed the importances were defined as ints, but doubles makes sense). Anyway, tests have been updated.

Nice! Only caveat here is that importances will now be truncated to 6 digits... something we should probably be aware of. The example here explains in more detail.

After reading a bit on this subject, it seems that our method in mcnp_funds.cpp for higher precision double --> string conversion (with the addition of a std::setprecision in the stream) is still recommended. 🤷‍♂️

I'd be surprised if more than 6 digits are used for these values very often, so I'm good w/ whatever you decide.

@gonuke or @makeclean - any thoughts on this?

I agree that the to_string() is not a good change

I agree as well -- we should go back to our _to_string function for floating point values. We can still use std::to_string for int's like material number though. Probably best to use the standard library where we can.

This sounds like a good idea- I will update accordingly.

I think to_string() will still convert ints to floats to write out (eg, 1 will become 1.000000 by default). But I might be able to play with the precision to avoid that.

You'll find that std::to_string() will write ints without trailing zeros. Its implementation is specialized depending on the type passed to it. So no extra work should be needed there 🎉

gonuke

Thanks for making these changes @kkiesling - just one question.

src/mcnp/tests/dagmcnp_unit_tests.cpp

pshriwise · 2020-03-28T23:04:07Z

src/mcnp/tests/dagmcnp_unit_tests.cpp

-                            "9 1 0.022 imp:n=1 imp:p=1 ",
-                            "12 0  imp:n=0 imp:p=0   $ graveyard",
-                            "13 0  imp:n=1 imp:p=1   $ implicit complement"
+  const char* expected[] = {"1 9 -12.0 imp:n=1.000000 imp:p=1.000000 ",


This isn't a comment from PullRequest, but I think it's worth updating and is in the spirit of this PR.

With C++11 we can now define these expected values using an initializer list rather than the variable-length char array we're using now like so:

std::vector<std::string> expected_lcad = {"1 9 -12.0 imp:n=1.000000 imp:p=1.000000 ", "2 9 -12.0 imp:n=1.000000 imp:p=1.000000 ", ...

Happens in a few places here I think.

Good idea. I'll search throughout and update.

gonuke · 2020-03-30T23:06:10Z

src/mcnp/mcnp_funcs.cpp

-        mat_num = _to_string(matnumber);
-        density = "-" + _to_string(material.density);
+        mat_num = std::to_string(matnumber);
+        density = "-" + std::to_string(material.density);


should we also use _to_string() here?

yes- I haven't gone back through to untangle all the to_string()/_to_string() uses yet

kkiesling · 2020-03-31T02:52:10Z

I think this is ready now as long as the checks pass.

ljacobson64 · 2020-03-31T22:24:29Z

src/mcnp/mcnp5/CMakeLists.txt

@@ -52,7 +52,6 @@ set(CMAKE_Fortran_FLAGS_RELWITHDEBINFO "-O1 -g")

 # C compiler flags
 if (CMAKE_C_COMPILER_ID STREQUAL "Intel")
-  #set(CMAKE_C_FLAGS "${CMAKE_Fortran_FLAGS} -mcmodel=medium")


The reason these commented lines are here is that they appear in the makefiles for native MCNP, but I needed to turn them off. I don't remember why.

Should they be left in and put in use? Or is it okay to remove these lines?

Ehhhh yeah let's just delete them

pshriwise

Thanks for getting us started on this @kkiesling!

gonuke · 2020-04-02T13:09:39Z

Thanks for kicking it off @kkiesling

kkiesling added 3 commits March 27, 2020 10:27

removed unused commented lines

5235412

changed to std::cerr when it's an error

3f8f85c

converted graveyard and vacuum strings to variables

a67913d

pshriwise reviewed Mar 27, 2020

View reviewed changes

gonuke requested changes Mar 28, 2020

View reviewed changes

src/mcnp/tests/dagmcnp_unit_tests.cpp Outdated Show resolved Hide resolved

pshriwise reviewed Mar 28, 2020

View reviewed changes

gonuke reviewed Mar 30, 2020

View reviewed changes

news file

e41d2ee

kkiesling force-pushed the mcnp-code-review branch from 2f6d63f to e41d2ee Compare March 31, 2020 02:38

kkiesling added 2 commits March 30, 2020 21:42

using std::to_string for ints

dc2e6c2

more accurate news file

3d79f25

ljacobson64 reviewed Mar 31, 2020

View reviewed changes

pshriwise approved these changes Mar 31, 2020

View reviewed changes

gonuke approved these changes Apr 2, 2020

View reviewed changes

gonuke merged commit 6a89166 into svalinn:develop Apr 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply code review changes to MCNP files #665

Apply code review changes to MCNP files #665

kkiesling commented Mar 27, 2020

pshriwise left a comment

pshriwise Mar 27, 2020 •

edited

Loading

kkiesling Mar 28, 2020

pshriwise Mar 28, 2020 •

edited

Loading

kkiesling Mar 28, 2020

pshriwise Mar 28, 2020

kkiesling Mar 29, 2020

gonuke Mar 29, 2020

pshriwise Mar 29, 2020

kkiesling Mar 30, 2020 •

edited

Loading

pshriwise Mar 30, 2020

gonuke left a comment

pshriwise Mar 28, 2020

kkiesling Mar 28, 2020

gonuke Mar 30, 2020

kkiesling Mar 31, 2020

kkiesling commented Mar 31, 2020

ljacobson64 Mar 31, 2020

kkiesling Apr 1, 2020 •

edited

Loading

ljacobson64 Apr 1, 2020

pshriwise left a comment

gonuke commented Apr 2, 2020

Apply code review changes to MCNP files #665

Apply code review changes to MCNP files #665

Conversation

kkiesling commented Mar 27, 2020

pshriwise left a comment

Choose a reason for hiding this comment

pshriwise Mar 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pshriwise Mar 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkiesling Mar 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gonuke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkiesling commented Mar 31, 2020

Choose a reason for hiding this comment

kkiesling Apr 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pshriwise left a comment

Choose a reason for hiding this comment

gonuke commented Apr 2, 2020

pshriwise Mar 27, 2020 •

edited

Loading

pshriwise Mar 28, 2020 •

edited

Loading

kkiesling Mar 30, 2020 •

edited

Loading

kkiesling Apr 1, 2020 •

edited

Loading