Adding console output methods to some classes #872

pshriwise · 2023-03-21T17:09:27Z

This is a step toward improved console logging in some of our classes. As described in #856, DAGMC and other classes in MOAB always write some output, which can conflict with output in downstream applications and cause confusion.

message, warning, and err_msg methods have been added to the dagmcMetaData and DagMC classes. Warnings and errors will always be written to screen. The message method allows a verbosity level to be associated with the message. The message will either be written to screen or not depending on the verbosity level set on the class instance. I've specified two levels of verbosity for now, but I'm open to a more fine grain set of verbosity levels down the line.

There's some duplicated code here, which could be improved. I'll look into abstracting these capabilieis into an object that both of the affected classes can inherit from perhaps.

Another thing I don't love about this is that a std::stringstream object needs to be created for messages where conversion of non-char/string types occurs via the streaming operator. There are other libraries like fmt that handle this more gracefully, but I'll leave that for an improvement in the future.

gonuke · 2023-03-21T18:01:15Z

src/dagmc/DagMC.cpp

@@ -290,7 +288,7 @@ ErrorCode DagMC::remove_graveyard() {
  // get the implicit complement, it's children will need updating
  EntityHandle implicit_complement = 0;
  rval = geom_tool()->get_implicit_complement(implicit_complement);
-  if (rval != MB_ENTITY_NOT_FOUND || rval != MB_SUCCESS) {


how did this ever work?

gonuke · 2023-03-21T18:07:41Z

Did you mean this to be extended from #855 (which we should get back to)?

pshriwise · 2023-03-21T18:35:30Z

Did you mean this to be extended from #855 (which we should get back to)?

Ugh no. Accidentally combined some stashed changes I think. Fixing...

gonuke · 2023-03-21T19:24:47Z

I've merged #855, so maybe it will be easy to just rebase on that?

gonuke · 2023-03-23T14:25:48Z

If you rebase we can probably more forward on this @pshriwise

gonuke

A few attempts to tidy... can't help but feel that there is some other external solution we can import

gonuke · 2023-04-04T11:43:02Z

src/dagmc/DagMC.hpp

+    return verbosity = val;
+  }
+
+  int get_verbosity(int val) { return verbosity; }


Why does this take an argument- seems confusing

gonuke · 2023-04-04T11:48:18Z

src/dagmc/DagMC.hpp

+  int set_verbosity(int val) {
+    int verbosity_max = 0;
+    int verbosity_min = 1;
+    if (val < verbosity_min || val > verbosity_max)
+      warning("Invalid verbosity value " + std::to_string(val) +
+              " will be set to nearest valid value.");
+    val = std::min(std::max(verbosity_min, val), verbosity_max);
+    return verbosity = val;


Unless my brain is missing something, this will always result in 0?

Mm yeah mentally bit-flipped the initializer values at the beginning of this function.

gonuke · 2023-04-04T11:49:41Z

src/dagmc/DagMC.hpp

+    if (lvl > verbosity) return;
+    // otherwise, display message
+    if (newline)
+      std::cout << msg << "\n";


Suggested change

std::cout << msg << "\n";

std::cout << msg << std::endl;

I think it's preferred to use a newline literal these days. Using std::endl will force the output buffer to flush and is more likely to cause garbled messages in concurrent applications, so I thought I'd start taking us in that direction.

Could do it all in one go in a separate changeset though.

I thought it was preferred because it is platform independent (e.g. Windows carriage returns...). That's the only reason I suggested it.

src/dagmc/dagmcmetadata.cpp

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

pshriwise · 2023-04-04T14:33:24Z

Thanks for the review @gonuke! Not sure why the Housekeeping/Linux tests are failing at this point. Something about git permissions in the CI image. Made an attempt at addressing it, but no luck yet. I'll try to revisit it later if I have time.

gonuke · 2023-04-04T20:32:00Z

The failing the linux builds is something annoying that I was sure we had resolved many months ago. Not sure why it's coming up again.

gonuke · 2023-04-04T20:35:00Z

I made a quick scan for logging libraries and didn't find anything compelling - all a little heavier weight than I'd like.

Do you think we could write a header-only small class that would do this and include it in the relevant places?

pshriwise · 2023-04-04T21:07:29Z

We certainly could! I don't have the cycles to do so at the moment though

gonuke · 2023-04-04T23:23:20Z

We certainly could! I don't have the cycles to do so at the moment though

Let me see how I might adapt what you have done in that way

gonuke · 2023-05-01T12:27:04Z

I think this was superseded by #876

ahnaf-tahmid-chowdhury · 2023-05-07T11:11:17Z

.github/workflows/linux_build_test.yml

@@ -49,7 +49,7 @@ jobs:
      - name: Checkout repository


I think this may fix the linux build test failure

Suggested change

- name: Checkout repository

- name: Add exception for dubious ownership

run: git config --global --add safe.directory /__w/DAGMC/DAGMC

- name: Checkout repository

Thanks, but I think this PR has been replaced by another ( #876 ), so will be close by @pshriwise soon.

Correct! Closing now.

pshriwise · 2023-05-07T14:03:42Z

Closing in favor of #876. @ahnaf-tahmid-chowdhury, if the changes you've suggested above apply in that PR as well would you mind adding them there also?

pshriwise force-pushed the verbosity branch from 9800599 to 78bf6e9 Compare March 21, 2023 17:20

gonuke reviewed Mar 21, 2023

View reviewed changes

pshriwise force-pushed the verbosity branch from fe2af3b to c6da794 Compare March 21, 2023 18:04

pshriwise force-pushed the verbosity branch from c6da794 to 4474d7f Compare March 21, 2023 18:35

pshriwise added 2 commits March 23, 2023 09:31

Adding console output methods to dagmcmetadata class as well

6a86654

Updating changelog

32cc94f

pshriwise force-pushed the verbosity branch from 4474d7f to 335b710 Compare March 23, 2023 14:31

pshriwise added 3 commits March 23, 2023 09:31

Correcting changelog

824e71d

Applying clang-format

335b710

Turn off submodule checkout in action and let CMake do it.

6086785

gonuke requested changes Apr 4, 2023

View reviewed changes

pshriwise and others added 4 commits April 4, 2023 08:47

Apply review suggestions from @gonuke

490b3bb

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

Updating get_verbosity signature and fixing set_verbosity

d390bf2

Housekeeping

bad74da

Safe dir

52e64a3

gonuke mentioned this pull request Apr 5, 2023

Simple logger #876

Merged

ahnaf-tahmid-chowdhury requested changes May 7, 2023

View reviewed changes

pshriwise closed this May 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding console output methods to some classes #872

Adding console output methods to some classes #872

pshriwise commented Mar 21, 2023

gonuke Mar 21, 2023

gonuke commented Mar 21, 2023 •

edited

Loading

pshriwise commented Mar 21, 2023

gonuke commented Mar 21, 2023

gonuke commented Mar 23, 2023

gonuke left a comment

gonuke Apr 4, 2023

gonuke Apr 4, 2023

pshriwise Apr 4, 2023

gonuke Apr 4, 2023

pshriwise Apr 4, 2023 •

edited

Loading

pshriwise Apr 4, 2023

gonuke Apr 4, 2023

pshriwise commented Apr 4, 2023

gonuke commented Apr 4, 2023

gonuke commented Apr 4, 2023

pshriwise commented Apr 4, 2023

gonuke commented Apr 4, 2023

gonuke commented May 1, 2023

ahnaf-tahmid-chowdhury May 7, 2023

gonuke May 7, 2023

pshriwise May 7, 2023

pshriwise commented May 7, 2023

Adding console output methods to some classes #872

Adding console output methods to some classes #872

Conversation

pshriwise commented Mar 21, 2023

Choose a reason for hiding this comment

gonuke commented Mar 21, 2023 • edited Loading

pshriwise commented Mar 21, 2023

gonuke commented Mar 21, 2023

gonuke commented Mar 23, 2023

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

pshriwise Apr 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pshriwise commented Apr 4, 2023

gonuke commented Apr 4, 2023

gonuke commented Apr 4, 2023

pshriwise commented Apr 4, 2023

gonuke commented Apr 4, 2023

gonuke commented May 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pshriwise commented May 7, 2023

gonuke commented Mar 21, 2023 •

edited

Loading

pshriwise Apr 4, 2023 •

edited

Loading