From 8c7e5b591deeb1d6762f9315df99245d2a4449e7 Mon Sep 17 00:00:00 2001 From: Baptiste Mouginot Date: Fri, 15 May 2020 11:25:00 -0500 Subject: [PATCH 1/5] addressing most of the PullRequest-Agent comments in src/dagmc folder astyling astyling dagMC.hpp this should do it this should finally be working --- news/PR-0676.rst | 12 ++++ src/dagmc/DagMC.cpp | 71 ++++++++++++++----- src/dagmc/DagMC.hpp | 18 +++-- src/dagmc/dagmcmetadata.cpp | 84 +++++++++++++---------- src/dagmc/dagmcmetadata.hpp | 7 +- src/dagmc/tests/dagmc_pointinvol_test.cpp | 2 +- src/dagmc/tests/dagmc_simple_test.cpp | 2 +- src/dagmc/tools/test_geom.cpp | 14 ++-- 8 files changed, 135 insertions(+), 75 deletions(-) create mode 100644 news/PR-0676.rst diff --git a/news/PR-0676.rst b/news/PR-0676.rst new file mode 100644 index 0000000000..f0aa6b1a5f --- /dev/null +++ b/news/PR-0676.rst @@ -0,0 +1,12 @@ +**Added:** None + +**Changed:** + - adressing comment by from PullRequest-Agent related to src/dagmc files + +**Deprecated:** None + +**Removed:** None + +**Fixed:** None + +**Security:** None diff --git a/src/dagmc/DagMC.cpp b/src/dagmc/DagMC.cpp index 76919858e3..af15494367 100644 --- a/src/dagmc/DagMC.cpp +++ b/src/dagmc/DagMC.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include @@ -20,6 +21,7 @@ #define MB_OBB_TREE_TAG_NAME "OBB_TREE" #define FACETING_TOL_TAG_NAME "FACETING_TOL" +static const int null_delimiter_length = 1; namespace moab { @@ -92,16 +94,22 @@ float DagMC::version(std::string* version_string) { unsigned int DagMC::interface_revision() { unsigned int result = 0; - const char* interface_string = DAGMC_INTERFACE_REVISION; - if (strlen(interface_string) >= 5) { + std::string interface_string = DAGMC_INTERFACE_REVISION; + if (interface_string.rfind("$Rev:") != std::string::npos) { // start looking for the revision number after "$Rev: " - result = strtol(interface_string + 5, NULL, 10); + char* endptr = NULL; + errno = 0; + result = strtol(interface_string.c_str() + 5, &endptr, 10); + if (endptr == interface_string.c_str() + 5 || + ((result == LONG_MAX || result == LONG_MIN) && errno == ERANGE)) { + std::cerr << "Not able to parse revision number" << std::endl; + exit(1); + } + return result; } - return result; + /* SECTION I: Geometry Initialization and problem setup */ } -/* SECTION I: Geometry Initialization and problem setup */ - // the standard DAGMC load file method ErrorCode DagMC::load_file(const char* cfile) { ErrorCode rval; @@ -111,7 +119,12 @@ ErrorCode DagMC::load_file(const char* cfile) { 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); + if (sizeof(cfile) / sizeof(char) > 4) { + memcpy(file_ext, &cfile[strlen(cfile) - 4], 4); + } else { + std::cerr << "DagMC warning: unhandled file " + << "loading options." << std::endl; + } EntityHandle file_set; rval = MBI->create_meshset(MESHSET_SET, file_set); @@ -124,7 +137,7 @@ ErrorCode DagMC::load_file(const char* cfile) { // Some options were unhandled; this is common for loading h5m files. // Print a warning if an option was unhandled for a file that does not end in '.h5m' std::string filename(cfile); - if (filename.length() < 4 || filename.substr(filename.length() - 4) != ".h5m") { + if (std::string(file_ext) != ".h5m") { std::cerr << "DagMC warning: unhandled file loading options." << std::endl; } } else if (MB_SUCCESS != rval) { @@ -185,7 +198,8 @@ ErrorCode DagMC::setup_obbs() { return MB_SUCCESS; } -// setups of the indices for the problem, builds a list of +// setups of the indices for the problem, builds a list of surface and volumes +// indices ErrorCode DagMC::setup_indices() { Range surfs, vols; ErrorCode rval = setup_geometry(surfs, vols); @@ -205,8 +219,6 @@ ErrorCode DagMC::init_OBBTree() { MB_CHK_SET_ERR(rval, "GeomTopoTool could not find the geometry sets"); // implicit compliment - // EntityHandle implicit_complement; - // rval = GTT->get_implicit_complement(implicit_complement, true); rval = setup_impl_compl(); MB_CHK_SET_ERR(rval, "Failed to setup the implicit compliment"); @@ -379,10 +391,21 @@ ErrorCode DagMC::build_indices(Range& surfs, Range& vols) { ErrorCode rval = MB_SUCCESS; // surf/vol offsets are just first handles - setOffset = std::min(*surfs.begin(), *vols.begin()); - - // max - EntityHandle tmp_offset = std::max(surfs.back(), vols.back()); + EntityHandle tmp_offset = 0; + + if (surfs.size() != 0 && vols.size() != 0) { + setOffset = std::min(*surfs.begin(), *vols.begin()); + tmp_offset = std::max(surfs.back(), vols.back()); + } else if (0 == surfs.size()) { + setOffset = *surfs.begin(); + tmp_offset = surfs.back(); + } else if (0 == vols.size()) { + setOffset = *vols.begin(); + tmp_offset = vols.back(); + } else { + std::cout << "Volumes or Surfaces founds" << std::endl; + return MB_ENTITY_NOT_FOUND; + } // set size entIndices.resize(tmp_offset - setOffset + 1); @@ -391,6 +414,9 @@ ErrorCode DagMC::build_indices(Range& surfs, Range& vols) { // index by handle lists surf_handles().resize(surfs.size() + 1); std::vector::iterator iter = surf_handles().begin(); + // 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. *(iter++) = 0; std::copy(surfs.begin(), surfs.end(), iter); int idx = 1; @@ -399,6 +425,10 @@ ErrorCode DagMC::build_indices(Range& surfs, Range& vols) { vol_handles().resize(vols.size() + 1); iter = vol_handles().begin(); + + // 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. *(iter++) = 0; std::copy(vols.begin(), vols.end(), iter); idx = 1; @@ -528,12 +558,13 @@ ErrorCode DagMC::append_packed_string(Tag tag, EntityHandle eh, } // append a new value for the property to the existing property string - unsigned int tail_len = new_string.length() + 1; - char* new_packed_string = new char[ len + tail_len ]; + unsigned int tail_len = new_string.length() + null_delimiter_length; + int new_len = tail_len + len; + + char* new_packed_string = new char[ new_len ]; memcpy(new_packed_string, str, len); memcpy(new_packed_string + len, new_string.c_str(), tail_len); - int new_len = len + tail_len; p = new_packed_string; rval = MBI->tag_set_by_ptr(tag, &eh, 1, &p, &new_len); delete[] new_packed_string; @@ -554,7 +585,7 @@ ErrorCode DagMC::unpack_packed_string(Tag tag, EntityHandle eh, while (idx < len) { std::string item(str + idx); values.push_back(item); - idx += item.length() + 1; + idx += item.length() + null_delimiter_length; } return MB_SUCCESS; } @@ -621,6 +652,8 @@ ErrorCode DagMC::parse_properties(const std::vector& keywords, const unsigned int groupsize = grp_sets.size(); for (unsigned int j = 0; j < groupsize; ++j) { rval = append_packed_string(proptag, grp_sets[j], groupval); + if (MB_SUCCESS != rval) + return rval; } } } diff --git a/src/dagmc/DagMC.hpp b/src/dagmc/DagMC.hpp index 5bb8ef2412..28e047821f 100644 --- a/src/dagmc/DagMC.hpp +++ b/src/dagmc/DagMC.hpp @@ -30,6 +30,11 @@ struct DagmcVolData { namespace moab { +static const int surfs_handle_idx = 2; +static const int vols_handle_idx = 3; +static const int groups_handle_idx = 4; + + class CartVect; /**\brief @@ -88,7 +93,7 @@ class DagMC { * * 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 meshsets have triangles as members, but OBB's as children + * operation are fair game, the surface meshsets have triangles as members, but OBBs as children * but no querying is done, just assumptions that the tags exist. */ ErrorCode load_file(const char* cfile); @@ -359,9 +364,12 @@ class DagMC { ErrorCode unpack_packed_string(Tag tag, EntityHandle eh, std::vector< std::string >& values); - std::vector& surf_handles() {return entHandles[2];} - std::vector& vol_handles() {return entHandles[3];} - std::vector& group_handles() {return entHandles[4];} + std::vector& surf_handles() + {return entHandles[surfs_handle_idx];} + std::vector& vol_handles() + {return entHandles[vols_handle_idx];} + std::vector& group_handles() + {return entHandles[groups_handle_idx];} Tag get_tag(const char* name, int size, TagType store, DataType type, const void* def_value = NULL, bool create_if_missing = true); @@ -446,7 +454,7 @@ inline int DagMC::index_by_handle(EntityHandle handle) { } inline unsigned int DagMC::num_entities(int dimension) { - assert(0 <= dimension && 3 >= dimension); + assert(0 <= dimension && groups_handle_idx >= dimension); return entHandles[dimension].size() - 1; } diff --git a/src/dagmc/dagmcmetadata.cpp b/src/dagmc/dagmcmetadata.cpp index 29463fdf80..f95d44c56d 100644 --- a/src/dagmc/dagmcmetadata.cpp +++ b/src/dagmc/dagmcmetadata.cpp @@ -3,6 +3,9 @@ #include #include +const char graveyard_str[] = "Graveyard"; +const char vacuum_str[] = "Vacuum"; + // constructor for metadata class dagmcMetaData::dagmcMetaData(moab::DagMC* dag_ptr, bool verbosity) { DAG = dag_ptr; // dagmc pointer @@ -32,7 +35,6 @@ void dagmcMetaData::load_property_data() { parse_boundary_data(); parse_tally_volume_data(); parse_tally_surface_data(); - // finalise_counters(); } // get the given volume property on a given entity handle @@ -74,7 +76,8 @@ std::string dagmcMetaData::get_surface_property(std::string property, moab::Enti } else if (property == "tally") { value = tally_data_eh[eh]; } else { - std::cout << "Not a valid property for surfaces" << std::endl; + std::cerr << property << " is not a valid property for surfaces" + << std::endl; } return value; } @@ -135,13 +138,13 @@ void dagmcMetaData::parse_material_data() { 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; - std::cout << "that does not the the _comp tag associated with it" << std::endl; - std::cout << cellid << " has the following material assignments" << std::endl; + std::cerr << "more than one material for volume with id " << cellid << std::endl; + std::cerr << "that does not the the _comp tag associated with it" << std::endl; + std::cerr << cellid << " has the following material assignments" << std::endl; for (int j = 0 ; j < material_props.size() ; j++) { - std::cout << material_props[j] << std::endl; + std::cerr << material_props[j] << std::endl; } - std::cout << "Please check your material assignments " << cellid << std::endl; + std::cerr << "Please check your material assignments " << cellid << std::endl; exit(EXIT_FAILURE); } } else { @@ -149,39 +152,41 @@ void dagmcMetaData::parse_material_data() { // at least one entry, "" if nothing is found // if there is no material property - not failure for impl_comp if (material_props[0] == "" && !(DAG->is_implicit_complement(eh))) { - std::cout << "No material property found for volume with ID " << cellid << std::endl; - std::cout << "Every volume must have only one mat: property" << std::endl; + std::cerr << "No material property found for volume with ID " << cellid << std::endl; + std::cerr << "Every volume must have only one mat: property" << std::endl; exit(EXIT_FAILURE); } } // this is never ok for a volume to have more than one proprety for density if (density_props.size() > 1) { - std::cout << "More than one density specified for " << cellid << std::endl; - std::cout << cellid << " has the following density assignments" << std::endl; + std::cerr << "More than one density specified for " << cellid << std::endl; + std::cerr << cellid << " has the following density assignments" << std::endl; for (int j = 0 ; j < density_props.size() ; j++) { - std::cout << density_props[j] << std::endl; + std::cerr << density_props[j] << std::endl; } - std::cout << "Please check your density assignments " << cellid << std::endl; + std::cerr << "Please check your density assignments " << cellid << std::endl; exit(EXIT_FAILURE); } std::string grp_name = ""; // determine if we have a density property - if (!density_props[0].empty()) { - grp_name = "mat:" + material_props[0] + "/rho:" + density_props[0]; - volume_density_data_eh[eh] = density_props[0]; - } else { - grp_name = "mat:" + material_props[0]; - volume_density_data_eh[eh] = ""; + if (density_props.size() == 1) { + if (!density_props[0].empty()) { + grp_name = "mat:" + material_props[0] + "/rho:" + density_props[0]; + volume_density_data_eh[eh] = density_props[0]; + } else { + grp_name = "mat:" + material_props[0]; + volume_density_data_eh[eh] = ""; + } } // check to see if the simplified naming scheme is used, by try to convert the // material property to an int if (try_to_make_int(material_props[0]) && density_props[0].empty() && !(DAG->is_implicit_complement(eh))) { - std::cout << "Using the simplified naming scheme without a density" << std::endl; - std::cout << "property is forbidden, please rename the group mat:" << material_props[0] << std::endl; + std::cerr << "Using the simplified naming scheme without a density" << std::endl; + std::cerr << "property is forbidden, please rename the group mat:" << material_props[0] << std::endl; exit(EXIT_FAILURE); } @@ -189,27 +194,26 @@ void dagmcMetaData::parse_material_data() { 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 + if (grp_name.find(graveyard_str) == std::string::npos && grp_name.find(vacuum_str) == std::string::npos && !(DAG->is_implicit_complement(eh))) { - // set the volume_material_data_eh[eh] = material_props[0]; } // found graveyard - else if (grp_name.find("Graveyard") != std::string::npos) { + else if (grp_name.find(graveyard_str) != std::string::npos) { volume_material_property_data_eh[eh] = "mat:Graveyard"; - volume_material_data_eh[eh] = "Graveyard"; + volume_material_data_eh[eh] = graveyard_str; } // vacuum - else if (grp_name.find("Vacuum") != std::string::npos) { + else if (grp_name.find(vacuum_str) != std::string::npos) { volume_material_property_data_eh[eh] = "mat:Vacuum"; - volume_material_data_eh[eh] = "Vacuum"; + volume_material_data_eh[eh] = vacuum_str; } // implicit complement else if (DAG->is_implicit_complement(eh)) { if (implicit_complement_material == "") { std::cout << "Implicit Complement assumed to be Vacuum" << std::endl; volume_material_property_data_eh[eh] = "mat:Vacuum"; - volume_material_data_eh[eh] = "Vacuum"; + volume_material_data_eh[eh] = vacuum_str; } else { volume_material_property_data_eh[eh] = "mat:" + implicit_complement_material; if (implicit_complement_density != "") @@ -237,7 +241,7 @@ void dagmcMetaData::parse_importance_data() { // vector of importance values importance_assignment = importance_assignments[eh]; - // set the value of each string for + // set the value of each string std::string importances = "|"; for (int j = 0 ; j < importance_assignment.size() ; j++) { if (importance_assignment[j] == "") @@ -250,7 +254,15 @@ void dagmcMetaData::parse_importance_data() { 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()); + char* e; + errno = 0; + double imp = std::strtod(pair.second.c_str(), &e); + if (*e != '\0' || errno != 0) { + std::cerr << "Can't parse " << pair.second << " as a float." + << std::endl; + exit(EXIT_FAILURE); + } + importance_map[eh][pair.first] = imp; } else { std::cout << "Volume with ID " << volid << " has more than one importance " << std::endl; std::cout << "Assigned for particle type " << pair.first << std::endl; @@ -296,7 +308,7 @@ void dagmcMetaData::parse_tally_volume_data() { // vector of tally values tally_assignment = tally_assignments[eh]; - // set the value of each string for + // set the value of each string std::string tally = "|"; for (int j = 0 ; j < tally_assignment.size() ; j++) { // delimit each particle/value pair with a pipe symbol @@ -402,7 +414,7 @@ std::map > dagmcMetaData::get_prope // being found upto the first delimiter if (delimiters.size() > 1) { for (int j = 0 ; j < properties.size() ; j++) { - size_t npos = 0, first = npos; + size_t npos = 0; npos = properties[j].find(delimiters[1]); // extract from the match - which is either first // match or .length() @@ -461,12 +473,12 @@ std::set dagmcMetaData::set_remove_rich(std::set prope std::set new_set = properties_set; std::vector::const_iterator> matches; - std::set::iterator set_it, toofar, it; std::set to_delete; // loop over all elements in the set - it = new_set.begin(); + std::set::iterator it = new_set.begin(); while (it != new_set.end()) { + std::set::iterator set_it, toofar; // loop over the set trying to find similar names for (set_it = new_set.begin(), toofar = new_set.end(); set_it != toofar; ++set_it) if ((*set_it).find(*it) != std::string::npos && (*set_it != *it)) { @@ -477,7 +489,7 @@ std::set dagmcMetaData::set_remove_rich(std::set prope // if there were more than 2 matches if (matches.size() > 1) { int smallest = 0; - int len = 1e7; + int len = std::numeric_limits::max(); for (int i = 0 ; i < matches.size() ; i++) { if ((*matches[i]).length() < len) { smallest = i; @@ -583,7 +595,7 @@ bool dagmcMetaData::try_to_make_int(std::string value) { // try to convert the string value into an int char* end; - int i = strtol(value.c_str(), &end, 10); + strtol(value.c_str(), &end, 10); if (*end == '\0') return true; else diff --git a/src/dagmc/dagmcmetadata.hpp b/src/dagmc/dagmcmetadata.hpp index ee8b83d71c..66f4857684 100644 --- a/src/dagmc/dagmcmetadata.hpp +++ b/src/dagmc/dagmcmetadata.hpp @@ -1,3 +1,5 @@ +#ifndef SRC_DAGMC_DAGMCMETADATA_HPP_ +#define SRC_DAGMC_DAGMCMETADATA_HPP_ #include #include #include "DagMC.hpp" @@ -78,12 +80,11 @@ class dagmcMetaData { // map of importance data std::map > importance_map; - // material density pairs - // std::map > material_density_pairs; - private: moab::DagMC* DAG; // Pointer to DAGMC instance bool verbose; std::vector< std::string > metadata_keywords; std::map< std::string, std::string > keyword_synonyms; }; + +#endif // SRC_DAGMC_DAGMCMETADATA_HPP_ diff --git a/src/dagmc/tests/dagmc_pointinvol_test.cpp b/src/dagmc/tests/dagmc_pointinvol_test.cpp index 314ad10c60..c545d5848d 100644 --- a/src/dagmc/tests/dagmc_pointinvol_test.cpp +++ b/src/dagmc/tests/dagmc_pointinvol_test.cpp @@ -57,7 +57,7 @@ int dagmc_point_in_vol_dir(double origin[3], double dir[3], int vol_idx) { double next_surf_dist; EntityHandle next_surf; - // normalise the vector + // normalise the direction vector double dir_norm = (dir[0] * dir[0]) + (dir[1] * dir[1]) + (dir[2] * dir[2]); dir[0] = dir[0] / sqrt(dir_norm); diff --git a/src/dagmc/tests/dagmc_simple_test.cpp b/src/dagmc/tests/dagmc_simple_test.cpp index a013c835d9..f019e20e35 100644 --- a/src/dagmc/tests/dagmc_simple_test.cpp +++ b/src/dagmc/tests/dagmc_simple_test.cpp @@ -218,7 +218,7 @@ TEST_F(DagmcSimpleTest, dagmc_rayfire) { } TEST_F(DagmcSimpleTest, dagmc_closest_to) { - const double eps = 1e-6; // epsilon for test, faceting tol? + const double eps = 1e-6; // epsilon for test, faceting tolerance int vol_idx = 1; // note model is cube of side 10, centred at 0,0,0, so ray fire along diff --git a/src/dagmc/tools/test_geom.cpp b/src/dagmc/tools/test_geom.cpp index e4a98681f7..86d0059d5a 100644 --- a/src/dagmc/tools/test_geom.cpp +++ b/src/dagmc/tools/test_geom.cpp @@ -293,14 +293,14 @@ static bool run_test(std::string name, int argc, char* argv[]) { return false; } -#define RUN_TEST(A) do { \ +#define RUN_TEST(A) { \ if (run_test( #A, argc, argv )) { \ std::cout << #A << "... " << std::endl; \ if (MB_SUCCESS != A ( dagmc ) ) { \ ++errors; \ } \ } \ - } while(false) + } int main(int argc, char* argv[]) { ErrorCode rval; @@ -322,7 +322,6 @@ int main(int argc, char* argv[]) { DagMC* dagmc = new DagMC(); int errors = 0; - //rval = dagmc.moab_instance()->load_file( filename ); rval = dagmc->load_file(filename); remove(filename); if (MB_SUCCESS != rval) { @@ -915,12 +914,6 @@ ErrorCode test_point_in_volume(DagMC* dagmc) { { { 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 - //{ {-1.0, 0.0, 0.0 }, BOUNDARY}, - //{ { 0.0, 1.0, 0.0 }, BOUNDARY}, - //{ { 0.0,-1.0, 0.0 }, BOUNDARY}, - //{ { 0.0, 0.0, 0.0 }, BOUNDARY}, - //{ { 0.0, 0.0,-1.0 }, BOUNDARY} }; const int num_test = sizeof(tests) / sizeof(tests[0]); ErrorCode rval; @@ -1114,7 +1107,8 @@ ErrorCode overlap_test_tracking(DagMC* dagmc) { 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) { + double expected_dist = 0.91; + if (next_surf != surfs[7] || fabs(dist - expected_dist) > 1e-6) { std::cerr << "ERROR: failed on advance 1" << std::endl; return MB_FAILURE; } From 9b6edc13e6d72f1dc4d47a7cbea8867649147c56 Mon Sep 17 00:00:00 2001 From: Baptiste Mouginot <15145274+bam241@users.noreply.github.com> Date: Mon, 18 May 2020 08:36:28 -0500 Subject: [PATCH 2/5] Apply suggestions from code review Co-authored-by: Paul Wilson --- src/dagmc/dagmcmetadata.cpp | 2 +- src/dagmc/tests/dagmc_pointinvol_test.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/dagmc/dagmcmetadata.cpp b/src/dagmc/dagmcmetadata.cpp index f95d44c56d..3b017df6db 100644 --- a/src/dagmc/dagmcmetadata.cpp +++ b/src/dagmc/dagmcmetadata.cpp @@ -258,7 +258,7 @@ void dagmcMetaData::parse_importance_data() { errno = 0; double imp = std::strtod(pair.second.c_str(), &e); if (*e != '\0' || errno != 0) { - std::cerr << "Can't parse " << pair.second << " as a float." + std::cerr << "Can't parse importance " << pair.second << " as a float." << std::endl; exit(EXIT_FAILURE); } diff --git a/src/dagmc/tests/dagmc_pointinvol_test.cpp b/src/dagmc/tests/dagmc_pointinvol_test.cpp index c545d5848d..a59bc786bc 100644 --- a/src/dagmc/tests/dagmc_pointinvol_test.cpp +++ b/src/dagmc/tests/dagmc_pointinvol_test.cpp @@ -57,7 +57,7 @@ int dagmc_point_in_vol_dir(double origin[3], double dir[3], int vol_idx) { double next_surf_dist; EntityHandle next_surf; - // normalise the direction vector + // direction vectors are always interpreted as unit vectors - make sure this one is normalized double dir_norm = (dir[0] * dir[0]) + (dir[1] * dir[1]) + (dir[2] * dir[2]); dir[0] = dir[0] / sqrt(dir_norm); From fb61a5cdede64516acd51d846b4d63e741ecd217 Mon Sep 17 00:00:00 2001 From: Baptiste Mouginot Date: Mon, 18 May 2020 09:31:34 -0500 Subject: [PATCH 3/5] adressing most of @gonuke comments adding comments on error checking the string atof changing strstod to stod and adding mechanisum to catch conversion exception astyling DagMC.cpp and dagmcmetadata.cpp astyling DagMC.hpp --- src/dagmc/DagMC.cpp | 42 ++++++++++++++++++------------------- src/dagmc/DagMC.hpp | 19 ++++++++++------- src/dagmc/dagmcmetadata.cpp | 12 +++++------ 3 files changed, 38 insertions(+), 35 deletions(-) diff --git a/src/dagmc/DagMC.cpp b/src/dagmc/DagMC.cpp index af15494367..a0f9b082c3 100644 --- a/src/dagmc/DagMC.cpp +++ b/src/dagmc/DagMC.cpp @@ -94,38 +94,43 @@ float DagMC::version(std::string* version_string) { unsigned int DagMC::interface_revision() { unsigned int result = 0; + std::string key_str = "$Rev:"; + int max_length = 10; + std::string interface_string = DAGMC_INTERFACE_REVISION; - if (interface_string.rfind("$Rev:") != std::string::npos) { + if (interface_string.rfind(key_str) != std::string::npos) { // start looking for the revision number after "$Rev: " char* endptr = NULL; errno = 0; - result = strtol(interface_string.c_str() + 5, &endptr, 10); - if (endptr == interface_string.c_str() + 5 || - ((result == LONG_MAX || result == LONG_MIN) && errno == ERANGE)) { + result = strtol(interface_string.c_str() + key_str.size(), &endptr, + max_length); + // check if the string has been fully parsed and the results is within + // normal range + if (endptr == interface_string.c_str() + key_str.size() // parsing end + || ((result == LONG_MAX || result == LONG_MIN) + && errno == ERANGE)) { // checking range std::cerr << "Not able to parse revision number" << std::endl; exit(1); } return result; } - /* SECTION I: Geometry Initialization and problem setup */ + return result; } // the standard DAGMC load file method ErrorCode DagMC::load_file(const char* cfile) { ErrorCode rval; + std::string filename(cfile); std::cout << "Loading file " << cfile << std::endl; // load options char options[120] = {0}; - char file_ext[4] = "" ; // file extension + std::string file_ext = "" ; // file extension // get the last 4 chars of file .i.e .h5m .sat etc - if (sizeof(cfile) / sizeof(char) > 4) { - memcpy(file_ext, &cfile[strlen(cfile) - 4], 4); - } else { - std::cerr << "DagMC warning: unhandled file " - << "loading options." << std::endl; + int file_extension_size = 4; + if (filename.size() > file_extension_size) { + file_ext = filename.substr(filename.size() - file_extension_size); } - EntityHandle file_set; rval = MBI->create_meshset(MESHSET_SET, file_set); if (MB_SUCCESS != rval) @@ -393,19 +398,12 @@ ErrorCode DagMC::build_indices(Range& surfs, Range& vols) { // surf/vol offsets are just first handles EntityHandle tmp_offset = 0; - if (surfs.size() != 0 && vols.size() != 0) { - setOffset = std::min(*surfs.begin(), *vols.begin()); - tmp_offset = std::max(surfs.back(), vols.back()); - } else if (0 == surfs.size()) { - setOffset = *surfs.begin(); - tmp_offset = surfs.back(); - } else if (0 == vols.size()) { - setOffset = *vols.begin(); - tmp_offset = vols.back(); - } else { + if (surfs.size() == 0 || vols.size() == 0) { std::cout << "Volumes or Surfaces founds" << std::endl; return MB_ENTITY_NOT_FOUND; } + setOffset = std::min(*surfs.begin(), *vols.begin()); + tmp_offset = std::max(surfs.back(), vols.back()); // set size entIndices.resize(tmp_offset - setOffset + 1); diff --git a/src/dagmc/DagMC.hpp b/src/dagmc/DagMC.hpp index 28e047821f..ee3d8c1b8d 100644 --- a/src/dagmc/DagMC.hpp +++ b/src/dagmc/DagMC.hpp @@ -30,6 +30,8 @@ struct DagmcVolData { namespace moab { +static const int vertex_handle_idx = 0; +static const int curve_handle_idx = 1; static const int surfs_handle_idx = 2; static const int vols_handle_idx = 3; static const int groups_handle_idx = 4; @@ -364,12 +366,15 @@ class DagMC { ErrorCode unpack_packed_string(Tag tag, EntityHandle eh, std::vector< std::string >& values); - std::vector& surf_handles() - {return entHandles[surfs_handle_idx];} - std::vector& vol_handles() - {return entHandles[vols_handle_idx];} - std::vector& group_handles() - {return entHandles[groups_handle_idx];} + std::vector& surf_handles() { + return entHandles[surfs_handle_idx]; + } + std::vector& vol_handles() { + return entHandles[vols_handle_idx]; + } + std::vector& group_handles() { + return entHandles[groups_handle_idx]; + } Tag get_tag(const char* name, int size, TagType store, DataType type, const void* def_value = NULL, bool create_if_missing = true); @@ -454,7 +459,7 @@ inline int DagMC::index_by_handle(EntityHandle handle) { } inline unsigned int DagMC::num_entities(int dimension) { - assert(0 <= dimension && groups_handle_idx >= dimension); + assert(vertex_handle_idx <= dimension && groups_handle_idx >= dimension); return entHandles[dimension].size() - 1; } diff --git a/src/dagmc/dagmcmetadata.cpp b/src/dagmc/dagmcmetadata.cpp index 3b017df6db..6e0e379834 100644 --- a/src/dagmc/dagmcmetadata.cpp +++ b/src/dagmc/dagmcmetadata.cpp @@ -254,12 +254,12 @@ void dagmcMetaData::parse_importance_data() { imp_particles.insert(pair.first); // insert into map too if (importance_map[eh].count(pair.first) == 0) { - char* e; - errno = 0; - double imp = std::strtod(pair.second.c_str(), &e); - if (*e != '\0' || errno != 0) { - std::cerr << "Can't parse importance " << pair.second << " as a float." - << std::endl; + double imp = 0; + try { + imp = std::stod(pair.second.c_str()); + } catch (const std::exception& e) { + std::cerr << "Can't parse importance " << pair.second + << " as a float: " << e.what() << std::endl; exit(EXIT_FAILURE); } importance_map[eh][pair.first] = imp; From aec23710b4a98102ab856d32ca0ebccda3efe993 Mon Sep 17 00:00:00 2001 From: Baptiste Mouginot <15145274+bam241@users.noreply.github.com> Date: Tue, 19 May 2020 08:33:19 -0500 Subject: [PATCH 4/5] Apply suggestions from code review Co-authored-by: Paul Wilson --- src/dagmc/DagMC.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/dagmc/DagMC.cpp b/src/dagmc/DagMC.cpp index a0f9b082c3..1ada0fb630 100644 --- a/src/dagmc/DagMC.cpp +++ b/src/dagmc/DagMC.cpp @@ -399,7 +399,7 @@ ErrorCode DagMC::build_indices(Range& surfs, Range& vols) { EntityHandle tmp_offset = 0; if (surfs.size() == 0 || vols.size() == 0) { - std::cout << "Volumes or Surfaces founds" << std::endl; + std::cout << "Volumes or Surfaces not found" << std::endl; return MB_ENTITY_NOT_FOUND; } setOffset = std::min(*surfs.begin(), *vols.begin()); From 4da86ca40d71705e43e7345ce0165d8aff8594f7 Mon Sep 17 00:00:00 2001 From: Baptiste Mouginot Date: Tue, 19 May 2020 08:37:24 -0500 Subject: [PATCH 5/5] Adressing remaining review comments addressing @gonuke last comments --- src/dagmc/DagMC.cpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/dagmc/DagMC.cpp b/src/dagmc/DagMC.cpp index 1ada0fb630..db5c0b0c3d 100644 --- a/src/dagmc/DagMC.cpp +++ b/src/dagmc/DagMC.cpp @@ -117,6 +117,8 @@ unsigned int DagMC::interface_revision() { return result; } +/* SECTION I: Geometry Initialization and problem setup */ + // the standard DAGMC load file method ErrorCode DagMC::load_file(const char* cfile) { ErrorCode rval; @@ -142,7 +144,7 @@ ErrorCode DagMC::load_file(const char* cfile) { // Some options were unhandled; this is common for loading h5m files. // Print a warning if an option was unhandled for a file that does not end in '.h5m' std::string filename(cfile); - if (std::string(file_ext) != ".h5m") { + if (file_ext != ".h5m") { std::cerr << "DagMC warning: unhandled file loading options." << std::endl; } } else if (MB_SUCCESS != rval) { @@ -395,15 +397,14 @@ int DagMC::get_entity_id(EntityHandle this_ent) { ErrorCode DagMC::build_indices(Range& surfs, Range& vols) { ErrorCode rval = MB_SUCCESS; - // surf/vol offsets are just first handles - EntityHandle tmp_offset = 0; if (surfs.size() == 0 || vols.size() == 0) { std::cout << "Volumes or Surfaces not found" << std::endl; return MB_ENTITY_NOT_FOUND; } setOffset = std::min(*surfs.begin(), *vols.begin()); - tmp_offset = std::max(surfs.back(), vols.back()); + // surf/vol offsets are just first handles + EntityHandle tmp_offset = std::max(surfs.back(), vols.back()); // set size entIndices.resize(tmp_offset - setOffset + 1);