Addressing most of the comments about src/dagmc files by bam241 · Pull Request #676 · svalinn/DAGMC

bam241 · 2020-05-13T17:01:13Z

This addresses most of the comments about dagmc sources from #660 .

I was not sure how to fix some of them so I ping @pshriwise @gonuke @makeclean on them to get more information.

I also included changes suggested by my linter (mostly line <80 chars...)

bam241 · 2020-05-13T17:28:21Z

I am thinking that addressing the linter suggestion in this PR might be a bit much...
Let me know I can simplify this if necessary....

gonuke

Some thoughts based on context and background.

gonuke · 2020-05-17T22:22:24Z

-  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) {


Does anyone know if this actually works!?!? This looks like a tag expansion that was part of CVS/SVN, but I don't think we ever get "$Rev:" in the file anymore. Perhaps we need a new solution for this entirely?

If it is working, maybe we should define $Rev: as a string variable, and refer to it's length where we otherwise use 5 as a magic number below.

I'm not sure why 10 as a magic number below?

sounds about right for $rev

I don't know what this 10 is for....

this comes from src/dagmc/DagMCVersion.hpp.in

and did not get populate in my DagMCVersion.hpp:
#define DAGMC_INTERFACE_REVISION "$Rev$" <- I still have this like in bld/src/dagmc/DagMCVersion.hpp

gonuke · 2020-05-17T22:29:03Z

+  if (sizeof(cfile) / sizeof(char) > 4) {
+    memcpy(file_ext, &cfile[strlen(cfile) - 4], 4);
+  } else {
+    std::cerr << "DagMC warning: unhandled file "


It might still be better for this error to be generated later, after any error messages that might get generated by load_file. It also looks like they create a std::string version of cfile - maybe we should do that earlier and take advantage of std::string functions throughout.

gonuke · 2020-05-17T22:36:38Z

-  EntityHandle tmp_offset = std::max(surfs.back(), vols.back());
+  EntityHandle tmp_offset = 0;
+
+  if (surfs.size() != 0 && vols.size() != 0) {


Looking back at the Pull-Request comment.... I am not sure if these checks are necessary. All valid DagMC geometries must have at least one surface and at least one volume. I don't know if there is a prior check to make sure this is true. If there is, then no check is needed here. If there is not, then perhaps we should check that both are non-zero length and give a failing error if either one is, and then use the (nearly) original code....

just wondering if only one of them is non zero would the min()/max() still work ?

gonuke · 2020-05-17T22:40:20Z


 inline unsigned int DagMC::num_entities(int dimension) {
-  assert(0 <= dimension && 3 >= dimension);
+  assert(0 <= dimension && groups_handle_idx >= dimension);


Since you've setup these variables, you might want to add:
vertex_handle_idx = 0 (to use here) and
curve_handle_idx = 1 (for completeness)

gonuke · 2020-05-17T22:43:06Z

-    } else {
-      grp_name = "mat:" + material_props[0];
-      volume_density_data_eh[eh] = "";
+    if (density_props.size() == 1) {


I wonder if it's possible for this to be valid if >1???

I do not know.

Would anyone would have with some more experience about this ?

1 is caught earlier on, line 102

ok I remember now,
162 ensure that density_props .size() <= 1
this ensure we have at lease 1 element before calling density_props[0]

gonuke · 2020-05-17T22:53:41Z

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


Wow - this is verbose. Why can't C++ make this easier??? Does std::stod do any better?

It probably can, IIRC we hadnt shifted off the C++03 standard for MOAB compatability, so going back to C99 was the only option

gonuke · 2020-05-17T23:05:32Z

Not sure why this is failing?? Might need a little more verbosity to figure it out.

gonuke

Maybe a couple of things lost in the revisions...

gonuke · 2020-05-19T03:33:41Z

  return result;
 }

-/* SECTION I: Geometry Initialization and problem setup */


Did we take out the other comments that for these Section headers?

I am not sure what I did with this...
Some kind of a weird combination of typos...

I'll restore it.

gonuke

Maybe a couple of things lost in the revisions...

gonuke

Two small requests & rebase for merge today

gonuke · 2020-05-28T11:34:09Z

    // 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") {


file_ext is already a std::string - no need to caste it here

gonuke · 2020-05-28T11:35:39Z


  // surf/vol offsets are just first handles
-  setOffset = std::min(*surfs.begin(), *vols.begin());
+  EntityHandle tmp_offset = 0;


Combine this with line 408

astyling astyling dagMC.hpp this should do it this should finally be working

Co-authored-by: Paul Wilson <paul.wilson@wisc.edu>

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

Co-authored-by: Paul Wilson <paul.wilson@wisc.edu>

@gonuke

addressing @gonuke last comments

bam241 · 2020-05-28T17:14:42Z

@gonuke I think this is ready.
I rebased, addressed your 2 remaining comments and cleaned the commit history (squash).

gonuke · 2020-05-28T17:27:42Z

Thanks @bam241 - finally merging!!!

bam241 requested review from gonuke and pshriwise May 13, 2020 17:06

bam241 mentioned this pull request May 13, 2020

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

Open

bam241 force-pushed the dagmc_PR_review branch from 83fd683 to 3453826 Compare May 15, 2020 16:28

gonuke requested changes May 17, 2020

View reviewed changes

gonuke requested changes May 19, 2020

View reviewed changes

gonuke requested changes May 28, 2020

View reviewed changes

bam241 force-pushed the dagmc_PR_review branch from 45fe862 to e29bc6c Compare May 28, 2020 15:58

bam241 and others added 5 commits May 28, 2020 12:09

addressing most of the PullRequest-Agent comments in src/dagmc folder

8c7e5b5

astyling astyling dagMC.hpp this should do it this should finally be working

Apply suggestions from code review

9b6edc1

Co-authored-by: Paul Wilson <paul.wilson@wisc.edu>

adressing most of @gonuke comments

fb61a5c

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

Apply suggestions from code review

aec2371

Co-authored-by: Paul Wilson <paul.wilson@wisc.edu>

Adressing remaining review comments

4da86ca

addressing @gonuke last comments

bam241 force-pushed the dagmc_PR_review branch from e29bc6c to 4da86ca Compare May 28, 2020 17:13

gonuke approved these changes May 28, 2020

View reviewed changes

gonuke merged commit bd4eb76 into svalinn:develop May 28, 2020

bam241 deleted the dagmc_PR_review branch November 28, 2024 14:39

Uh oh!

Conversation

bam241 commented May 13, 2020

Uh oh!

bam241 commented May 13, 2020

Uh oh!

gonuke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bam241 May 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gonuke commented May 17, 2020

Uh oh!

gonuke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gonuke left a comment

Choose a reason for hiding this comment

Uh oh!

gonuke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bam241 commented May 28, 2020

Uh oh!

gonuke commented May 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bam241 May 18, 2020 •

edited

Loading