PullRequest Full Project Review Comments [2020-01-09] #660

PullRequest-Agent · 2020-01-20T16:54:00Z

PR generated for professional code review sponsored by UKAEA

docker/Dockerfile

PullRequest-Agent · 2020-01-20T17:36:58Z

docker/Dockerfile

+
+# Update core packages
+RUN apt-get -y update
+RUN apt-get -y upgrade


Unfortunately, it's against best practices to run apt-get -y upgrade or apt-get -y dist-upgrade from within a docker container. From the dockerfile documentation [1]:

Avoid RUN apt-get upgrade and dist-upgrade, as many of the “essential” packages from the parent images cannot upgrade inside an unprivileged container. If a package contained in the parent image is out-of-date, contact its maintainers. If you know there is a particular package, foo, that needs to be updated, use apt-get install -y foo to update automatically.

[1] https://docs.docker.com/develop/develop-images/dockerfile_best-practices/#run

PullRequest-Agent · 2020-01-20T17:36:58Z

docker/Dockerfile

+                       python-numpy python-pip python-setuptools wget
+
+# Get astyle
+RUN if [ "${UBUNTU_VERSION}" == "18.04" ]; then \


Optional: I would recommend just having the 18.04 condition here. Dockerfile specifications tend to be operating system-version specific. It is probably better to fail fast here instead of trying to compensate.

PullRequest-Agent · 2020-01-20T17:36:59Z

docker/build_dagmc.sh

@@ -0,0 +1,32 @@
+#!/bin/bash
+
+set -e


Optional: For dockerfiles, I often also set -x so it will print the commands as it runs [1]. This is helpful in debugging bash files when it fails. You can set both as follows:

set -ex

[1] https://www.gnu.org/software/bash/manual/html_node/The-Set-Builtin.html#The-Set-Builtin

docker/build_dagmc.sh

docker/build_geant4.sh

docker/build_moab.sh

docker/travis.housekeeping.sh

docker/travis.tests.sh

PullRequest-Agent · 2020-01-20T17:37:03Z

src/uwuw/uwuw.cpp

+  hid_t arr_space = H5Dget_space(ds);
+
+  hsize_t arr_dims[1];
+  int arr_ndim = H5Sget_simple_extent_dims(arr_space, arr_dims, NULL);


arr_ndim is not used and can be omitted.

PullRequest-Agent · 2020-01-20T17:37:03Z

src/uwuw/uwuw_preprocessor.cpp

+  std::vector<std::string>::iterator it;
+
+  // first volumes
+  for (int i = 1 ; i <= DAG->num_entities(3); i++) {


int should be replaced with size_t.

PullRequest-Agent · 2020-01-20T17:37:04Z

src/uwuw/uwuw_preprocessor.cpp

+    }
+  }
+  // now surfaces
+  for (int i = 1 ; i <= DAG->num_entities(2); i++) {


int should be replaced with size_t.

PullRequest-Agent · 2020-01-20T17:37:04Z

src/uwuw/uwuw_preprocessor.cpp

+    if (verbose || fatal) {
+      std::cout << "More than one material for volume with id " << cellid << std::endl;
+      std::cout << cellid << " has the following material assignments" << std::endl;
+      for (int j = 0 ; j < material_props.size() ; j++) {


int should be replaced with size_t.

PullRequest-Agent · 2020-01-20T17:37:04Z

src/uwuw/uwuw_preprocessor.cpp

+    if (verbose || fatal) {
+      std::cout << "More than one density specified for " << cellid << std::endl;
+      std::cout << cellid << " has the following density assignments" << std::endl;
+      for (int j = 0 ; j < density_props.size() ; j++) {


int should be replaced with size_t.

PullRequest-Agent · 2020-01-20T17:37:05Z

src/uwuw/uwuw_preprocessor.cpp

+void uwuw_preprocessor::property_vector(std::vector<int> props) {
+  if (props.size() == 0)
+    return;
+  for (int i = 0 ; i < props.size() ; i++) {


int should be replaced with size_t.

PullRequest-Agent · 2020-01-20T17:37:05Z

src/uwuw/uwuw.cpp

+}
+
+// see if file exists
+bool UWUW::check_file_exists(std::string filename) {


I've noticed many functions throughout this code base where an object instance, typically a string or vector, is passed by value when it could be passed by reference, especially const reference. eg. this function could be changed to have the following signature for greater performance, optimization potential, and guarantee against modification of the passed-in string:

bool UWUW::check_file_exists(const std::string& filename);

PullRequest-Agent · 2020-01-20T17:37:05Z

src/uwuw/uwuw_preprocessor.cpp

+    material_name = "mat:" + material.metadata["name"].asString() + "/rho:" + density;
+    new_mat.density = atof(density.c_str());
+  } else {
+    material_name = "mat:" + material.metadata["name"].asString();


This if-else could be simplified a little by doing the following part only once:

material_name = "mat:" + material.metadata["name"].asString();

PullRequest-Agent · 2020-01-20T17:37:06Z

src/uwuw/uwuw_preprocessor.cpp

+}
+
+// used for printing & debugging only
+void uwuw_preprocessor::property_vector(std::vector<int> props) {


A const& to the props vector would be better.

PullRequest-Agent · 2020-01-20T17:37:06Z

src/uwuw/uwuw_preprocessor.cpp

+
+
+// constructor
+name_concatenator::name_concatenator() {


Depending on optimization level, it could be better to have these empty bodies in the header file so that when objects are created/destroyed it is known that no additional code needs to be called out to.

PullRequest-Agent · 2020-01-20T17:37:09Z

src/tally/KDEMeshTally.cpp

+      num_ebins--;
+
+    double tally_vect[num_ebins];
+    double error_vect[num_ebins];


Variable length arrays are not supported in C++, though since they are in newer versions of C many C++ compilers support them. I'll suggest the changes required to use vectors instead here, but it's not necessarily required to use them. For these two lines, change them to:

std::vector<double> tally_vect(num_ebins); std::vector<double> error_vect(num_ebins);

One more set of changes (two lines) below.

PullRequest-Agent · 2020-01-20T17:37:09Z

src/tally/KDEMeshTally.cpp

+    rval = mbi->tag_set_data(tally_tag, &point, 1, tally_vect);
+    MB_CHK_SET_ERR_RET(rval, "Failed to set the tally_tag data");
+    rval = mbi->tag_set_data(error_tag, &point, 1, error_vect);
+    MB_CHK_SET_ERR_RET(rval, "Failed to set the error_tag data");


Change the two array references by appending calls to vector::data(), ie.:

rval = mbi->tag_set_data(tally_tag, &point, 1, tally_vect.data()); rval = mbi->tag_set_data(error_tag, &point, 1, error_vect.data());

PullRequest-Agent · 2020-01-20T17:37:10Z

src/tally/TrackLengthMeshTally.cpp

+      num_ebins--;
+
+    double tally_vect[num_ebins];
+    double error_vect[num_ebins];


Variable length arrays are not supported in C++, though since they are in newer versions of C many C++ compilers support them. I'll suggest the changes required to use vectors instead here, but it's not necessarily required to use them. For these two lines, change them to:

std::vector<double> tally_vect(num_ebins); std::vector<double> error_vect(num_ebins);

One more set of changes (two lines) below.

PullRequest-Agent · 2020-01-20T17:37:10Z

src/tally/TrackLengthMeshTally.cpp

+    rval = mb->tag_set_data(tally_tag, &t, 1, tally_vect);
+    MB_CHK_SET_ERR_RET(rval, "Failed to set tally_tag " + std::to_string(rval) + " " + std::to_string(t));
+    rval = mb->tag_set_data(error_tag, &t, 1, error_vect);
+    MB_CHK_SET_ERR_RET(rval, "Failed to set error_tag " + std::to_string(rval) + " " + std::to_string(t));


Change the two array references by appending calls to vector::data(), ie.:

rval = mb->tag_set_data(tally_tag, &t, 1, tally_vect.data()); rval = mb->tag_set_data(error_tag, &t, 1, error_vect.data());

PullRequest-Agent · 2020-01-20T17:37:10Z

src/tally/TrackLengthMeshTally.cpp

+void TrackLengthMeshTally::compute_tracklengths(const TallyEvent& event,
+                                                unsigned int ebin, double weight,
+                                                const std::vector<double>& intersections,
+                                                const std::vector<EntityHandle>& triangles) {


The argument triangles is not used within this function.

PullRequest-Agent · 2020-01-20T17:37:11Z

src/tally/tests/test_KDEKernel.cpp

+  // not implemented
+  std::string get_kernel_name() const {}
+  int get_order() const {}
+  int get_min_quadrature(unsigned int i) const {}


These should each return a value:

std::string get_kernel_name() const {return std::string();} int get_order(return 0;) const {} int get_min_quadrature(unsigned int i) const {return (int)i;}

PullRequest-Agent · 2020-01-20T17:37:11Z

src/tally/tests/test_KDENeighborhood.cpp

+  moab::Interface* mbi = &mb_core;
+
+  // load default mesh
+  moab::ErrorCode rval = mbi->load_mesh("structured_mesh.h5m");


rval isn't used, so:

/*moab::ErrorCode rval =*/ mbi->load_mesh("structured_mesh.h5m");

It's used on the next line.

PullRequest-Agent · 2020-01-20T17:37:11Z

src/overlap_check/ProgressBar.hpp

+
+ public:
+  // constructor
+  ProgressBar();


Constructor not required as initialization occurs via class member initializers, so this can be removed along with the implementation in the .cpp file.

The constructor does other work, so this recommendation is not supported.

PullRequest-Agent · 2020-01-20T17:37:12Z

src/overlap_check/ProgressBar.hpp

+#ifndef DAGMC_PROGRESSBAR_H
+#define DAGMC_PROGRESSBAR_H
+
+#include <string>


<string> header not referenced and can be omitted.

Moved this to the CPP file to reduce header creep

PullRequest-Agent · 2020-01-20T17:37:12Z

src/overlap_check/ProgressBar.hpp

+
+ private:
+  int current {0};
+};


Suggest addition of bool need_final_newline {false};. See ProgressBar.cpp for more details. This is to remove superfluous newline on successful completion [part 1 of 4].

PullRequest-Agent · 2020-01-20T17:37:16Z

src/overlap_check/ProgressBar.cpp

+ProgressBar::ProgressBar() {
+  // initialize bar
+  set_value(0.0);
+}


Constructor is not needed due to initialization occurring via class member initializers, so these 4 lines can be removed along with the declaration in the header file.

Inline this with an initializer list to retain the explicit clarity that we are initializing to 0

disagree with removing the constructor since it accomplished a number of things.

PullRequest-Agent · 2020-01-20T17:37:17Z

src/overlap_check/ProgressBar.cpp

+}
+
+ProgressBar::~ProgressBar() {
+  std::cout << "\n";


Suggest replacing single line in destructor with the following:

if (need_final_newline) { std::cout << "\n"; }

This is to remove superfluous newline on successful completion [part 2 of 4].

PullRequest-Agent · 2020-01-20T17:37:17Z

src/overlap_check/ProgressBar.cpp

+  bar << "|+";
+
+  // write the bar to screen
+  std::cout << '\r' << bar.str() << std::flush;


Suggest adding this after the std::cout write:

need_final_newline = true;

This is to remove superfluous newline on successful completion [part 3 of 4].

PullRequest-Agent · 2020-01-20T17:37:17Z

src/overlap_check/ProgressBar.cpp

+  // write the bar to screen
+  std::cout << '\r' << bar.str() << std::flush;
+  if (val >= 100.0) {
+    std::cout << "\n";


Suggest adding this inside the if-statement body:

need_final_newline = false;

This is to remove superfluous newline on successful completion [part 4 of 4].

PullRequest-Agent · 2020-01-20T17:37:18Z

src/overlap_check/app/overlap_check.cpp

+
+  std::string filename;
+  int points_per_tri_edge {0};
+  int n_threads {0};


This should be guarded as so to remove the unused variable for single-threaded builds:

#ifdef _OPENMP int n_threads {0}; #endif

PullRequest-Agent · 2020-01-20T17:37:18Z

src/overlap_check/overlap.cpp

+  #pragma omp parallel shared(overlap_map, num_checked)
+  {
+    #pragma omp for schedule(auto)
+    for (int i = 0; i < all_verts.size(); i++) {


int should be replaced with size_t here. At a minimum this will remove a compiler warning about mismatched signed comparison against unsigned type, but it will also guard against problems that are difficult to track down if this is ever built on a less conventional platform.

PullRequest-Agent · 2020-01-20T17:37:19Z

src/overlap_check/overlap.cpp

+
+      EntityHandle vert = all_verts[i];
+      CartVect loc;
+      rval = MBI->get_coords(&vert, 1, loc.array());


rval is not used and should be dropped.

PullRequest-Agent · 2020-01-20T17:37:19Z

src/overlap_check/overlap.cpp

+    // (curve edges are likely in here too,
+    //  but it isn't hurting anything to check more locations)
+    #pragma omp for schedule(auto)
+    for (int i = 0; i < all_edges.size() ; i++) {


int should be replaced with size_t here. At a minimum this will remove a compiler warning about mismatched signed comparison against unsigned type, but it will also guard against problems that are difficult to track down if this is ever built on a less conventional platform.

PullRequest-Agent · 2020-01-20T17:37:19Z

src/overlap_check/test/overlap_check_test.hpp

+class OverlapTest : public::testing::Test {
+ protected:
+  virtual void SetUp();
+  virtual void TearDown();


Both of these functions should be marked with override. eg.:

virtual void Setup() override; virtual void TearDown() override;

This is more of a discipline that is good to follow as it makes it more likely that mistakes with virtual/overridden functions will result in compiler errors, making them easier to address.

PullRequest-Agent · 2020-01-20T17:37:20Z

src/overlap_check/test/overlap_check_test.cpp

+};
+
+class OverlappingVolumesTest : public OverlapTest {
+  virtual void SetFilename() { filename = "overlap.h5m"; }


This and all other SetFilename functions below should be marked with override. eg.:

virtual void SetFilename() override { filename = "overlap.h5m"; }

This is more of a discipline that is good to follow as it makes it more likely that mistakes with virtual/overridden functions will result in compiler errors, making them easier to address.

src/mcnp/mcnp5/CMakeLists.txt

src/mcnp/mcnp_funcs.cpp

PullRequest-Agent · 2020-01-20T17:37:25Z

src/mcnp/mcnp_funcs.cpp

+  moab::ErrorCode result = DAG->ray_fire(vol, point, dir,
+                                         next_surf, next_surf_dist, &history,
+                                         (use_dist_limit ? dist_limit : 0)
+#ifdef ENABLE_RAYSTAT_DUMPS


Readibility: Probably a bit cleaner to put a variable inside the #ifdef with a value, then put that variable here as a parameter, rather than break up the function parameters with a #ifdef.

To be able to move this outside of the function call, there needs to be a rework of the variables and how they are defined throughout. The variable raystat_dump is only defined if ENABLE_RAYSTAT_DUMPS is on.

src/mcnp/mcnp_funcs.cpp

src/make_watertight/Arc.cpp

PullRequest-Agent · 2020-01-20T17:37:30Z

src/make_watertight/Arc.hpp

+
+  /// goes through curve_sets and finds any curves with coincident ( dist. apart <= FACET_TOL) front and back points.
+  /// it then merges the curves topologically. Any merged curves aren't deleted until prepare surfaces.
+  moab::ErrorCode merge_curves(moab::Range curve_sets, const double FACET_TOL,


Readability: It's very unusual to have a non-class-level static constant identifier, especially a parameter, as all upper-case (FACET_TOL). Recommend making this lower case unless it has special scientific or other significance.

src/make_watertight/CMakeLists.txt

src/make_watertight/CheckWatertight.cpp

src/make_watertight/CheckWatertight.hpp

src/make_watertight/Cleanup.cpp

src/make_watertight/Gen.cpp

src/make_watertight/Gen.hpp

src/make_watertight/MakeWatertight.cpp

PullRequest-Agent · 2020-01-20T18:08:55Z

src/geant4/DagSolid.cc

+//
+// %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
+
+//#include "G4TessellatedSolid.hh"


Readability: Remove commented out code.

Here and throughout this file.

PullRequest-Agent · 2020-01-20T18:08:55Z

src/geant4/DagSolid.cc

+
+  fdagmc = dagmc;
+  fvolID = volID;
+  fvolEntity = fdagmc->entity_by_index(3, volID);


Optional: Typically you want the constructor of a class to have a very low surface area for errors, to avoid having high compute load or exceptions happen during initialization. Constructors are called by default when doing common operations like copying vectors of values, so getting this right can have a distinct impact on performance.

A elegant, well-encapsulated constructor typically consists of just declarations and straight value assignments.

Consequently, actions like lookups into pointer objects, for-loops to copy data, etc, are best initiated at the call site or in a subsequent init() method that can be called after the constructor is initiated.

I've also seen builder patterns used to great effect [1], or having static member functions that can be called with a constructed object to do the compute-heavy work post-initialization.

In this specific case, I would leverage the default constructor and turn this into a static intializer, rather than having it be an alternative constructor.

[1] https://sourcemaking.com/design_patterns/builder/cpp/1

PullRequest-Agent · 2020-01-20T18:08:55Z

src/geant4/DagSolid.hh

+
+
+
+//    virtual void ComputeDimensions (G4VPVParameterisation* p, const G4int n,


Readability: Remove commented out code. Here and throughout the file.

PullRequest-Agent · 2020-01-20T18:08:56Z

src/geant4/app/exampleN01.cc

+  ExN01ActionInitialization* actionInitialization = new ExN01ActionInitialization(workflow_data);
+  runManager->SetUserInitialization(actionInitialization);
+
+  //  G4VUserPrimaryGeneratorAction* gen_action = new ExN01PrimaryGeneratorAction;


Readability: Remove commented out code, here and throughout the file.

I was actually thinking about keeping those, as this is an example file ?
any thought @makeclean @gonuke @pshriwise ?

I guess I'd lean toward removing them unless there are comments added explaining why they're there for the purposes of the example. Other comments in this file seem to be explaining what each step in the example is doing, but here it seems like these were commented to ensure the example works.

I think this file is modified from a version that used to be distributed with Geant4, so it might be instructive to leave them in. However, this file is no longer distributed with Geant4, so probably not continued value.... Maybe better to create a new example based on a new standard Geant4 example, but that's out of scope for here. I would also require comments describing why these lines would be commented out in a DAGMC version

maybe this is particularly poor timing (and poor naming) but the example01.cc (and everything else present) really feature as a complete DAG-Geant4 program, linking with uwuw for materials (and tallies in some cases). There is likely some value in refactor, to factor out DagMaterial.cc and DagTally.cc (IIRC) as libraries for handling G4 materials and G4 tally objects.

PullRequest-Agent · 2020-01-20T18:08:56Z

src/geant4/app/include/ExN01DetectorHit.hh

+  void SetTrackID(G4int track)      {
+    fTrackID = track;
+  };
+  /*


Remove commented out code, here and throughout the file.

PullRequest-Agent · 2020-01-20T18:08:59Z

src/geant4/app/src/DagSolidMaterial.cc

+}
+
+// using each g4element make the geant4 version of each pyne material
+std::map<std::string, G4Material*> get_g4materials(std::map<int, G4Element*> element_map,


Note: I wasn't able to trace all the uses of get_g4materials up the call chain, but just a heads up that since this std::map contains pointers that are declared using new (on the heap), the ultimate owner of this data structure is responsible for explicitly delete'ing these values to avoid memory leaks.

[1] https://stackoverflow.com/a/19970551/1015951

PullRequest-Agent · 2020-01-20T18:09:00Z

src/geant4/app/src/DagSolidTally.cc

+  std::cout << filepath << std::endl;
+  while (!end) {
+    pyne::Tally tally; // from file
+    std::cout << i << std::endl;


I would add a label to this std::cout so the user knows what is being counted.

PullRequest-Agent · 2020-01-20T18:09:01Z

src/geant4/app/src/ExN01DetectorConstruction.cc

+  //  load tallies from the h5m file
+  tally_library = workflow_data->tally_library;
+
+  // TrackLength Scorer


Remove commented out code, here and throughout the file. More on line 414

PullRequest-Agent · 2020-01-20T18:09:01Z

src/geant4/app/src/ExN01DetectorConstruction.cc

+  }
+  end_histogram();
+
+  HM->print_histogram_collection();


Memory Leak: HM should be delete'd, here and for any other return values for this method.

PullRequest-Agent · 2020-01-20T18:09:01Z

src/geant4/app/src/ExN01DetectorConstruction.cc

+  // but CI doesnt like using the C++11 std
+  std::vector<G4double> bin_bounds_v;
+  // hence this
+  bin_bounds_v.assign(bin_bounds, bin_bounds + 261);


I would declare 261 a constant and use it in both places (here and in bin_bounds above), or use size_of.

PullRequest-Agent · 2020-01-20T18:09:02Z

src/geant4/app/src/ExN01DetectorHit.cc

+ExN01DetectorHit::ExN01DetectorHit()
+  : G4VHit(),
+    fTrackID(-1),
+    //fChamberNb(-1),


Same as above re: removing commented out code. Here and throughout.

PullRequest-Agent · 2020-01-20T18:09:02Z

src/fluka/fluka_funcs.cpp

+      state.history.reset();
+    }
+  }
+  /*


Readability: Remove commented out code, or document under what conditions it can be removed.
Here and throughout the file.

removing it.

@gonuke @makeclean @pshriwise, any idea if this is a safe deletion ?

Yes - I think this commented out code is just the logical equivalent of the code that is now above.

PullRequest-Agent · 2020-01-20T18:09:03Z

src/fluka/fluka_funcs.cpp

+
+  if (next_surf == 0) {   // if next_surface is 0 then we are lost
+    if (debug) {
+      std::cout << "!!! Lost Particle !!! " << std::endl;


If this is an error, might be worth it to switch to using std::cerr rather than std::cout

there are reasons to leave this alone due to the (fortran) units to which fluka writes arent clear if it uses std:error - but they are fundamentally correct

PullRequest-Agent · 2020-01-20T18:09:03Z

src/fluka/fluka_funcs.cpp

+  }
+
+  // set position to some large value
+  state.old_position[0] = 9.e99;


If you are looking to max out this value (assuming it is a double), you can use std::numeric_limits<double>::max().

[1] https://stackoverflow.com/a/5834704/1015951

PullRequest-Agent · 2020-01-20T18:09:04Z

src/fluka/fluka_funcs.cpp

+    if (is_inside == 1) {   // we are inside the cell tested
+      // test point in vol again, but reverse the direction
+      double new_dir[3];
+      new_dir[0] = -1.*dir[0];


Here and throughout, there are a lot of points in the code where the three axis of a point are managed via a double[3].

May be helpful and more readable to create a struct or helper class to manage and encapsulate the point / linear algebra math logic. This class can also provide special accessor and ingestors to interact with DAG.

Encapsulating this logic can lead to less future bugs and easier readability, as well as making the code less intimidating to newcomers overall.

PullRequest-Agent · 2020-01-20T18:09:07Z

src/fluka/fluka_funcs.cpp

+      return;
+    } else if (is_inside == -1) {
+      std::cout << "We cannot be here" << std::endl;
+      exit(0);


If this is an error, I would write this error to std::cerr and call exit(1). Typical POSIX executable exit codes are (0 = success, anything else = failure)

PullRequest-Agent · 2020-01-20T18:09:07Z

src/fluka/tests/fludag_unit_tests.cpp

+//---------------------------------------------------------------------------//
+// Test distance to next surface or vertex for various directions
+TEST_F(FluDAGTest, GFireGoodPropStep) {
+  // std::cout << "Calling g_fire. Start in middle of leftmost cube" << std::endl;


Remove commented out code, here and throughout the file.

PullRequest-Agent · 2020-01-20T18:09:08Z

src/dagmc/CMakeLists.txt

@@ -0,0 +1,21 @@
+message("")


Optional: Rather than having a blank message, I would state here that the dagmc CMake is starting. Or remove the message entirely.

src/dagmc/DagMC.cpp

PullRequest-Agent · 2020-01-20T18:09:08Z

src/dagmc/DagMC.cpp

+unsigned int DagMC::interface_revision() {
+  unsigned int result = 0;
+  const char* interface_string = DAGMC_INTERFACE_REVISION;
+  if (strlen(interface_string) >= 5) {


Optional: It may be prudent to use string::rfind [1] to check if $Rev: exists in the string. To be super robust, you could also create a regex [2] to locate $Rev: and explicitly check if it is followed by a number

[1] http://www.cplusplus.com/reference/string/string/rfind/
[2] http://www.cplusplus.com/reference/regex/

PullRequest-Agent · 2020-01-20T18:09:09Z

src/dagmc/DagMC.cpp

+  const char* interface_string = DAGMC_INTERFACE_REVISION;
+  if (strlen(interface_string) >= 5) {
+    // start looking for the revision number after "$Rev: "
+    result = strtol(interface_string + 5, NULL, 10);


Error Handling: If strtol returns a 0, it may have failed to parse the number successfully from the string. Specifically, if strtol returns 0 and errno is set to ERANGE, it means the number failed to parse. It may be worth it to check for this error state here.

[1] http://www.cplusplus.com/reference/cstdlib/strtol/

PullRequest-Agent · 2020-01-20T18:09:09Z

src/dagmc/DagMC.cpp

+  char file_ext[4] = "" ; // file extension
+
+  // get the last 4 chars of file .i.e .h5m .sat etc
+  memcpy(file_ext, &cfile[strlen(cfile) - 4], 4);


Error Handling: Recommend checking strlen on cfile here to avoid potentially passing a negative value as an array index.

PullRequest-Agent · 2020-01-20T18:09:09Z

src/dagmc/DagMC.cpp

+  return MB_SUCCESS;
+}
+
+// setups of the indices for the problem, builds a list of


Incomplete comment: Comment ends with "builds a list of". A few words likely got truncated off the end, likely "surfaces" or "volumes".

PullRequest-Agent · 2020-01-20T18:09:10Z

src/dagmc/DagMC.cpp

+  MB_CHK_SET_ERR(rval, "GeomTopoTool could not find the geometry sets");
+
+  // implicit compliment
+  // EntityHandle implicit_complement;


Recommend deleting commented out code. Optionally, you can leave an additional comment labeling under what condition the code can be re-instated or deleted.

@pshriwise @gonuke @makeclean @kkiesling I am not sure how to handle this one, so I'll just remove this. let me know if this is not a good idea.

I think that's fine. We've moved the notion of the implicit complement into the GTT...

PullRequest-Agent · 2020-01-20T18:09:10Z

src/dagmc/DagMC.cpp

+  ErrorCode rval = MB_SUCCESS;
+
+  // surf/vol offsets are just first handles
+  setOffset = std::min(*surfs.begin(), *vols.begin());


Error checking: Might be prudent to do a length check on surfs and vols here before getting these offsets.

PullRequest-Agent · 2020-01-20T18:09:13Z

src/dagmc/DagMC.cpp

+  // index by handle lists
+  surf_handles().resize(surfs.size() + 1);
+  std::vector<EntityHandle>::iterator iter = surf_handles().begin();
+  *(iter++) = 0;


Readability: For clarity, it may be helpful to leave an explicit comment explaining why the first value pointed to by the iterator is unconditionally set to 0.

@gonuke @pshriwise just to be sure I understand what is done there,
the gold here is to build a vector of handle to make correspondance between indices and handles number for Volumes Surfaces and Group ?
Is that correct ?

This is correct. The reason for this line is that MCNP wants a 1-based index but C++ has a 0-based index. So we need to set the first value to 0 and then start at the next position in the vector (iter++) thereafter. A comment is probably useful since it took me a few minutes to remember what was happening here.

PullRequest-Agent · 2020-01-20T18:09:13Z

src/dagmc/DagMC.cpp

+  for (Range::iterator rit = surfs.begin(); rit != surfs.end(); ++rit)
+    entIndices[*rit - setOffset] = idx++;
+
+  vol_handles().resize(vols.size() + 1);


Rather than explicitly pulling the iterator, setting it to 0, and incrementing it, it might be more readable to use the technique shown with group_handles() on line 409. E.g.:

vol_handles().resize(vols.size() + 1); vol_handles()[0] = 0; std::copy(vols.begin(), vols.end(), &vol_handles()[1]);

PullRequest-Agent · 2020-01-20T18:09:14Z

src/dagmc/DagMC.cpp

+  }
+
+  // append a new value for the property to the existing property string
+  unsigned int tail_len = new_string.length() + 1;


Readability: If the + 1 is being appended here to compensate for the NULL at the end of char* new_packed_string, it would be slightly more readable to calculate new_len here, explicitly add + 1, then declare new_packed_string as new char[ new_len ]

PullRequest-Agent · 2020-01-20T18:09:14Z

src/dagmc/DagMC.cpp

+  while (idx < len) {
+    std::string item(str + idx);
+    values.push_back(item);
+    idx += item.length() + 1;


Readability: By my read, the packed strings are separated by a sentinel of some type which requires a + 1 offset? maybe a NULL? If so, I would document it here for readability.

@pshriwise @gonuke any idea about this ?
I don't know enough about how the string are packed here to do the right thing...

See the comment in line 502 - they are packed in a single string separated by NULL which is how line 544 even works: the new std::string is initialized with a char* and reads to the NULL character

instead of a comment, you could add a line:

Suggested change

idx += item.length() + 1;

int null_delimiter_length = 1;

idx += item.length() + null_delimiter_length;

I'll add that into my PR #676

PullRequest-Agent · 2020-01-20T18:09:14Z

src/dagmc/DagMC.cpp

+        Tag proptag = property_tagmap[groupkey];
+        const unsigned int groupsize = grp_sets.size();
+        for (unsigned int j = 0; j < groupsize; ++j) {
+          rval = append_packed_string(proptag, grp_sets[j], groupval);


Should rval be checked here? Otherwise it looks like rval is never used and is overridden for every iteration of this loop.

PullRequest-Agent · 2020-01-20T18:09:15Z

src/dagmc/DagMC.hpp

+   *
+   * 1) The file is loaded and when we query the meshset, we find entities with the OBB_TREE tag
+   * 2) The OBBTreeTool assumes that any children of the entity being queried in a ray intersect sets
+   *     operation are fair game, the surface meshesets have triangles as members, but OBB's as children


Grammar: but OBB's as children but no querying is done -> that OBBs have children but no querying is done?

PullRequest-Agent · 2020-01-20T18:09:15Z

src/dagmc/DagMC.hpp

+class DagMC {
+ public:
+  // Constructor
+  DagMC(Interface* mb_impl = NULL, double overlap_tolerance = 0., double numerical_precision = .001);


Optional: Similar to how const char* is used on line 89, it may be valuable to use const when declaring other method parameters, especially pointers. This makes it easier for the compile to find accidental re-assignment of parameter values.

PullRequest-Agent · 2020-01-20T18:09:15Z

src/dagmc/DagMC.hpp

+  ErrorCode unpack_packed_string(Tag tag, EntityHandle eh,
+                                 std::vector< std::string >& values);
+
+  std::vector<EntityHandle>& surf_handles() {return entHandles[2];}


Readability: Would make it more readable to define 2, 3, and 4 as used here as static const int or #DEFINE types, with the identifiers indicating they are related to surfaces, volumes, and groups.

These can also be reused on line 425 below, when doing assert checks.

PullRequest-Agent · 2020-01-20T18:09:16Z

src/dagmc/dagmcmetadata.cpp

+  parse_boundary_data();
+  parse_tally_volume_data();
+  parse_tally_surface_data();
+  // finalise_counters();


Recommend removing commented out code, or leaving a note indicating under what conditions it can be removed.

PullRequest-Agent · 2020-01-20T18:09:16Z

src/dagmc/dagmcmetadata.cpp

+  } else if (property == "tally") {
+    value = tally_data_eh[eh];
+  } else {
+    std::cout << "Not a valid property for surfaces" << std::endl;


Best Practices: I would recommend sending this to std::cerr rather than std::cout. Additionally, I would recommend printing the invalid property in the error, to help the user with the invalid input.

PullRequest-Agent · 2020-01-20T18:09:18Z

src/dagmc/dagmcmetadata.cpp

+        material_props.erase(material_props.begin() + position);
+      } else {
+        // failure a volume can only have a single material associated with it
+        std::cout << "more than one material for volume with id " << cellid << std::endl;


Same comment as above re: std::cout vs std::cerr. Recommend using std::cerr here so it is more easily caught by logging aggregators and shell formatters.

PullRequest-Agent · 2020-01-20T18:09:19Z

src/dagmc/dagmcmetadata.cpp

+    std::string grp_name = "";
+
+    // determine if we have a density property
+    if (!density_props[0].empty()) {


Validity Checking: In addition to checking that density_props is at least 1 on line 159, it may also make sense to check if density_props is not empty before density_props[0] is indexed here.

PullRequest-Agent · 2020-01-20T18:09:19Z

src/dagmc/dagmcmetadata.cpp

+    volume_material_property_data_eh[eh] = grp_name;
+
+    // not graveyard or vacuum or implicit compliment
+    if (grp_name.find("Graveyard") == std::string::npos && grp_name.find("Vacuum") == std::string::npos


Reducing future errors: May make sense to define "Graveyard" and "Vacuum" somewhere as constants, rather than using the strings directly here and on line 198 below. Centralizing the string in a constant rather than using "magic strings" so-to-speak stops subtle bugs from potential misspellings later.

PullRequest-Agent · 2020-01-20T18:09:20Z

src/dagmc/dagmcmetadata.cpp

+    // not graveyard or vacuum or implicit compliment
+    if (grp_name.find("Graveyard") == std::string::npos && grp_name.find("Vacuum") == std::string::npos
+        && !(DAG->is_implicit_complement(eh))) {
+      // set the


Grammar: Incomplete comment (// set the ...)

PullRequest-Agent · 2020-01-20T18:09:20Z

src/dagmc/dagmcmetadata.cpp

+    // vector of importance values
+    importance_assignment = importance_assignments[eh];
+
+    // set the value of each string for


Incomplete comment: (// set the value of each string for ...)

PullRequest-Agent · 2020-01-20T18:09:20Z

src/dagmc/dagmcmetadata.cpp

+      imp_particles.insert(pair.first);
+      // insert into map too
+      if (importance_map[eh].count(pair.first) == 0) {
+        importance_map[eh][pair.first] = atof(pair.second.c_str());


I would recommend using std::strtod rather than atof, for better handling of std::string and error state [1].

[1] https://stackoverflow.com/a/12369101

PullRequest-Agent · 2020-01-20T18:09:21Z

src/dagmc/dagmcmetadata.cpp

+
+    // vector of tally values
+    tally_assignment = tally_assignments[eh];
+    // set the value of each string for


Incomplete comment: (// set the value of each string for)

PullRequest-Agent · 2020-01-20T18:09:21Z

src/dagmc/dagmcmetadata.cpp

+}
+
+
+// for a given property with a range of delimiters, and dimensionality


Potential incomplete comment: Did you mean , get property assignments and dimensionality rather than , and dimensionality?

PullRequest-Agent · 2020-01-20T18:09:21Z

src/dagmc/dagmcmetadata.cpp

+  std::set<std::string> new_set = properties_set;
+
+  std::vector<std::set<std::string>::const_iterator> matches;
+  std::set<std::string>::iterator set_it, toofar, it;


Best Practices: It would be better to declare set_it and toofar within the brace scope of their usage. In this case, this would be within the while loop starting on line 469.

This helps handle accidental re-assignment of variables, and also allows the compiler to better optimize variable usage to within the declared scope.

PullRequest-Agent · 2020-01-20T18:09:22Z

src/dagmc/dagmcmetadata.cpp

+    // if there were more than 2 matches
+    if (matches.size() > 1) {
+      int smallest = 0;
+      int len = 1e7;


Best Practices: If you are looking to initialize len with the maximum possible value of int, recommend using the INT_MAX constant. This is more readable and also is compiler specific based on the bit-width of int.

PullRequest-Agent · 2020-01-20T18:09:24Z

src/dagmc/dagmcmetadata.cpp

+        }
+      }
+      // take the longest
+      to_delete.insert(*matches[smallest]);


Potential Bug: I noticed here you're only inserting the smallest (or shortest) match into the to_delete set. Was it your intent to delete all matches that are not the longest (which may be more than just the smallest / shortest)? If so, you should correct that behavior here.

@gonuke @pshriwise @makeclean any idea about the indented behavior in here ?

I'm not super familiar with this, but from the comment at the top of the method, I think it assumes (perhaps incorrectly) that only two possible versions are possible tag_name and tag_name/value. We may want to clarify or enforce this in some way.

PullRequest-Agent · 2020-01-20T18:09:25Z

src/dagmc/dagmcmetadata.hpp

+  std::map<moab::EntityHandle, std::map<std::string, double> > importance_map;
+
+  // material density pairs
+  // std::map<std::string, std::set<std::string> > material_density_pairs;


Best practices: Remove commented-out code, or document under what condition it can be removed.

PullRequest-Agent · 2020-01-20T18:09:25Z

src/dagmc/tests/dagmc_pointinvol_test.cpp

+  // normalise the vector
+  double dir_norm = (dir[0] * dir[0]) + (dir[1] * dir[1]) + (dir[2] * dir[2]);
+
+  dir[0] = dir[0] / sqrt(dir_norm);


Best practices: I would create another double[3] here (maybe "dir_normalized"), rather than reassigning the parameter. Additionally, based on the current usages of dagmc_point_in_vol_dir, it doesn't look like the callers expect the parameters to be modified.

PullRequest-Agent · 2020-01-20T18:09:25Z

src/dagmc/tests/dagmc_pointinvol_test.cpp

+  double next_surf_dist;
+  EntityHandle next_surf;
+
+  // normalise the vector


Readability: I would add a note here indicating why the vector needs to be normalized in this case.

PullRequest-Agent · 2020-01-20T18:09:26Z

src/dagmc/tests/dagmc_simple_test.cpp

+}
+
+TEST_F(DagmcSimpleTest, dagmc_rayfire) {
+  const double eps = 1e-6; // epsilon for test, faceting tol?


Spelling: tol? May be an acronym.

PullRequest-Agent · 2020-01-20T18:09:26Z

src/dagmc/tests/dagmc_unit_tests.cpp

+  }
+
+  virtual void TearDown() {
+    //    delete dgm;


Potential Memory Leak: It looks like dgm is allocated (new'ed) for each test. It might be best to re-enable this code and delete it here.

If dgm is managed another way, it would be best to delete this commented-out code.

PullRequest-Agent · 2020-01-20T18:09:27Z

src/dagmc/tools/ray_fire_test.cpp

+  exit(error ? 1 : 0);
+}
+
+static const char* get_option(int& i, int argc, char* argv[]) {


Giving Information: There are a number of argument parsing libraries that help eliminate some of this boilerplate [1][2]. Personally I'm a fan of argparse [3], however getopt also I know is good [4].

[1] https://attractivechaos.wordpress.com/2018/08/31/a-survey-of-argument-parsing-libraries-in-c-c/.
[2] https://stackoverflow.com/questions/865668/how-to-parse-command-line-arguments-in-c
[3] https://github.com/hbristow/argparse
[4] http://www.gnu.org/savannah-checkouts/gnu/libc/manual/html_node/Getopt.html#Getopt

PullRequest-Agent · 2020-01-20T18:09:27Z

src/dagmc/tools/test_geom.cpp

+  DagMC* dagmc = new DagMC();
+
+  int errors = 0;
+  //rval = dagmc.moab_instance()->load_file( filename );


Would recommend removing commented out code here. Also on lines 368, 692, 849

PullRequest-Agent · 2020-01-20T18:09:27Z

src/dagmc/tools/test_geom.cpp

+  return false;
+}
+
+#define RUN_TEST(A) do { \


Looks like you have a do / while(false) loop in this macro that is effectively inert other than a brace scope change? If the purpose of this is to just create a brace level, you should be able to do that with just empty braces.

PullRequest-Agent · 2020-01-20T18:09:28Z

src/dagmc/tools/test_geom.cpp

+    { { 0.5, -0.5, -2.0 }, OUTSIDE, { 0.0, 0.0, 1.0} }
+  };
+
+  //    { { 1.0, 0.0, 0.0 }, BOUNDARY}, MCNP doesn't return on boundary


I would remove these commented-out test fixtures.

PullRequest-Agent · 2020-01-20T18:09:30Z

src/dagmc/tools/test_geom.cpp

+  DagMC::RayHistory history;
+  rval = dagmc->ray_fire(vol, point, dir, next_surf, dist, &history);
+  CHKERR;
+  if (next_surf != surfs[7] || fabs(dist - 0.91) > 1e-6) {


Readability: I would leave a comment explaining the value of 0.91.

@pshriwise @gonuke @makeclean any idea about that one ?

This is basically a unit test, so that is the expected value of dist... perhaps

Suggested change

if (next_surf != surfs[7] || fabs(dist - 0.91) > 1e-6) {

expected_dist = 0.91

if (next_surf != surfs[7] || fabs(dist - expected_dist) > 1e-6) {

included in #676

PullRequest-Agent · 2020-01-20T18:09:31Z

src/build_obb/build_obb.cpp

+  // sets the output filename if none specified
+  if (out_file == "") {
+    int pos = dag_file.find(".h5m");
+    out_file = dag_file.substr(0, pos) + "_obb.h5m";


Might be worth it to check here if pos is -1, indicating that .h5m has not been found.

PullRequest-Agent · 2020-01-20T18:09:31Z

misc/tests/test_uwuw_preproc_model.py

+    for k in pseudo_list:
+        if ("mat:" in k):
+            index = k.index('\n')
+            k = k[0:index]


Best practices: Normally it's preferred to not reassign loop incriminators unless you want to modify loop behavior. I would use another variable other than k here.

doc/usersguide/tools.rst

Addressing a number of miscellaneous changes from #660

adding back files to be reviewed

398fff7

PullRequest-Agent commented Jan 20, 2020

View reviewed changes

kkiesling mentioned this pull request Mar 27, 2020

Apply code review changes to MCNP files #665

Merged

pshriwise added a commit to pshriwise/DAGMC that referenced this pull request Apr 1, 2020

Updates based on comments from PullRequest in svalinn#660.

c03ed79

pshriwise mentioned this pull request Apr 1, 2020

Apply code review changes to make_watertight files #666

Merged

This was referenced Apr 2, 2020

Addressing docker comments #668

Merged

[Review NOT Required][CI-Testing][WIP]Addressing Dagmc.Xpp comments #670

Closed

Change pointer to shared_ptr #671

Merged

pshriwise mentioned this pull request Apr 11, 2020

Updates to make_watertight variables #672

Merged

bam241 mentioned this pull request May 13, 2020

Addressing most of the comments about src/dagmc files #676

Merged

gonuke mentioned this pull request Jun 11, 2020

Addressing a number of miscellaneous changes from #660 #680

Merged

pshriwise added a commit that referenced this pull request Jun 29, 2020

Merge pull request #680 from gonuke/660_uwuw_tally

bbc7785

Addressing a number of miscellaneous changes from #660

bam241 mentioned this pull request Jul 7, 2020

Addressing PullRequest comments related to Geant4 #691

Merged




		// virtual void ComputeDimensions (G4VPVParameterisation* p, const G4int n,

	idx += item.length() + 1;
	int null_delimiter_length = 1;
	idx += item.length() + null_delimiter_length;

		}


		// for a given property with a range of delimiters, and dimensionality

	if (next_surf != surfs[7] \|\| fabs(dist - 0.91) > 1e-6) {
	expected_dist = 0.91
	if (next_surf != surfs[7] \|\| fabs(dist - expected_dist) > 1e-6) {



		// constructor
		name_concatenator::name_concatenator() {

PullRequest Full Project Review Comments [2020-01-09] #660

Are you sure you want to change the base?

PullRequest Full Project Review Comments [2020-01-09] #660

Conversation

PullRequest-Agent commented Jan 20, 2020 • edited

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

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

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

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

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

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

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

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

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

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

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

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

PullRequest-Agent commented Jan 20, 2020 •

edited