Add macro for consistent sunrealtype formatting #517

Steven-Roberts · 2024-06-21T00:46:25Z

Replaces #498 after v7.1.0 update

src/sundials/sundials_utils.h

src/ida/ida_io.c

src/nvector/parhyp/nvector_parhyp.c

src/nvector/parallel/nvector_parallel.c

src/nvector/openmpdev/nvector_openmpdev.c

gardner48 · 2024-06-28T16:49:27Z

test/answers

Need to open a PR in the answers repo to merge these updates into main

test/unit_tests/arkode/CXX_serial/ark_test_dahlquist_mri_-1.out

include/sundials/sundials_linearsolver.h

include/sundials/sundials_nonlinearsolver.h

include/sundials/sundials_types.h

Co-authored-by: David Gardner <[email protected]>

…feature/real-format3

include/sundials/sundials_types.h

gardner48 · 2024-06-30T06:15:24Z

include/sundials/sundials_types.h

 #if defined(SUNDIALS_SINGLE_PRECISION)

 typedef float sunrealtype;
 #define SUN_RCONST(x)     x##F
 #define SUN_BIG_REAL      FLT_MAX
 #define SUN_SMALL_REAL    FLT_MIN
 #define SUN_UNIT_ROUNDOFF FLT_EPSILON
+#define SUN_FORMAT_E      "% ." SUN_STRING(FLT_DIG) "e"
+#define SUN_FORMAT_G      "% ." SUN_STRING(FLT_DIG) "g"


Looking at the updated outputs with "% ." I think it would be better to have this macro not include % (i.e., use #define SUN_FORMAT_G "." SUN_STRING(FLT_DIG) "g". This way the space after % can be added locally in the format string when it's needed for alignment (i.e., outputting matrix/vector entries) and in other cases (like PrintAllStats) the space can be excluded.

I agree the new output file format is not ideal with double spaces for positive numbers. What about this?

#define SUN_FORMAT_E "% ." SUN_STRING(FLT_DIG) "e" #define SUN_FORMAT_G "%." SUN_STRING(FLT_DIG) "g"

Note I just removed a space from the g format. We can use SUN_FORMAT_E for cases where alignment is needed (I think this is already the case), and SUN_FORMAT_G elsewhere. We could even rename them to SUN_FORMAT_ALIGNED and SUN_FORMAT. I like the simplicity and consistency of just two macros which contain the entire format specifier. Let's discuss at tomorrow's meeting.

Steven-Roberts · 2024-12-17T21:58:33Z

Aside from updating outfiles, I think this is ready. Before I spend the time updating all of those (just logging test out files updated for now) which will make the PR diff enormous, I wanted to see if there's any suggestions on the format, name of macros, etc.

drreynolds

Some initial feedback on the PR. Overall I think this is really helpful, but I have a few questions.

src/arkode/arkode.c

drreynolds · 2024-12-18T01:20:30Z

src/arkode/arkode_arkstep_io.c

@@ -1290,132 +1290,63 @@ int arkStep_PrintAllStats(ARKodeMem ark_mem, FILE* outfile, SUNOutputFormat fmt)
  retval = arkStep_AccessStepMem(ark_mem, __func__, &step_mem);
  if (retval != ARK_SUCCESS) { return (retval); }

-  switch (fmt)
+  /* function evaluations */
+  sunfprintf_long(outfile, fmt, SUNFALSE, "Explicit RHS fn evals", step_mem->nfe);


I like how the new sunfprintf functions really tighten things up with the table-vs-csv output, but it would be nice if these were documented somewhere (e.g., in the developer guide), since it took me a while to discover their definitions inside src/sundials/sundials_utils.h.

Also, have the outputs from these been compared with the previous outputs (especially for the CSV)?

I like how the new sunfprintf functions really tighten things up with the table-vs-csv output

Credit to @gardner48 from an earlier incarnation of this PR

it would be nice if these were documented somewhere

Agreed

Also, have the outputs from these been compared with the previous outputs (especially for the CSV)?

I haven't. Looks like there's some test that use CSV output, so I'll check those diffs.

it would be nice if these were documented somewhere

Agreed

The documentation could go in this section of the developer guide.

Also, have the outputs from these been compared with the previous outputs (especially for the CSV)?

I haven't. Looks like there's some test that use CSV output, so I'll check those diffs.

I checked this manually in the initial implementation but we should add a unit test to catch any regressions

src/arkode/arkode_butcher.c

drreynolds · 2024-12-18T01:49:50Z

swig/sundials/fsundials_types.i

+%ignore SUN_FORMAT_E;
+%ignore SUN_FORMAT_G;
+


Could these %ignore have undesirable consequences, e.g., if Swig ends up processing a header file that includes a print statement that uses one of these?

The format macros are only used in src/**/*.c files, so I don't think it should be a problem. No compilation issues for me. Any concerns @balos1?

test/unit_tests/logging/test_logging_arkode_arkstep_lvl5_0.out

include/sundials/sundials_types.h

gardner48 · 2024-12-18T05:10:00Z

src/arkode/arkode_arkstep_io.c

@@ -1290,132 +1290,63 @@ int arkStep_PrintAllStats(ARKodeMem ark_mem, FILE* outfile, SUNOutputFormat fmt)
  retval = arkStep_AccessStepMem(ark_mem, __func__, &step_mem);
  if (retval != ARK_SUCCESS) { return (retval); }

-  switch (fmt)
+  /* function evaluations */
+  sunfprintf_long(outfile, fmt, SUNFALSE, "Explicit RHS fn evals", step_mem->nfe);


it would be nice if these were documented somewhere

Agreed

The documentation could go in this section of the developer guide.

Also, have the outputs from these been compared with the previous outputs (especially for the CSV)?

I haven't. Looks like there's some test that use CSV output, so I'll check those diffs.

I checked this manually in the initial implementation but we should add a unit test to catch any regressions

src/arkode/arkode_butcher.c

src/arkode/arkode_erkstep_io.c

src/arkode/arkode_lsrkstep.c

src/sundials/sundials_utils.h

test/unit_tests/logging/test_logging_arkode_arkstep_lvl5_0.out

gardner48 · 2024-12-18T06:11:51Z

Aside from updating outfiles, I think this is ready. Before I spend the time updating all of those (just logging test out files updated for now) which will make the PR diff enormous, I wanted to see if there's any suggestions on the format, name of macros, etc.

The updateOutFiles.py script can help with updating the files. It will copy the .out files for any failed tests in a CI artifact to a given directory:

Download the output_files.zip artifact from the CI job
Unzip the file to get a directory named something like build_single_32_both_ON_DEV_7c819c41fc23
Copy the files to the desired location: ./scripts/updateOutFiles.py ~/Downloads/build_single_32_both_ON_DEV_7c819c41fc23 test/answers/linux-ubuntu20.04-x86_64/gcc-9.4.0/single

Co-authored-by: David Gardner <[email protected]>

gardner48

Some minor doc formatting items otherwise this looks good to me

doc/superbuild/source/developers/source_code/Style.rst

Co-authored-by: David Gardner <[email protected]>

Squash commits

3f3be17

Steven-Roberts requested review from balos1, gardner48, cswoodward and drreynolds as code owners June 21, 2024 00:46

gardner48 added 4 commits June 21, 2024 21:10

Merge branch 'develop' into feature/real-format3

39b1620

Merge branch 'develop' into feature/real-format3

c32face

update .out files

83905c8

Merge branch 'develop' into feature/real-format3

65e9da0

gardner48 requested changes Jun 28, 2024

View reviewed changes

Steven-Roberts and others added 8 commits June 28, 2024 12:10

Apply suggestions from code review

50f32bb

Co-authored-by: David Gardner <[email protected]>

Update src/sundials/sundials_utils.h

fe7f76d

Co-authored-by: David Gardner <[email protected]>

Remove final newline from matrix output

84024ab

Merge branch 'feature/real-format3' of github.com:LLNL/sundials into …

494f217

…feature/real-format3

Switch IDA to sunfprintf_*

bdb5bbb

Switch to SUN_FORMAT_*

df13a2a

Switch to 'e' format for MRI tables

817ed11

Apply formatter

a415ce9

gardner48 reviewed Jun 29, 2024

View reviewed changes

include/sundials/sundials_types.h Outdated Show resolved Hide resolved

Steven-Roberts and others added 4 commits June 28, 2024 21:08

Use space consistently in format

9b654df

Fix ida real stats

e9c9a71

Switch Butcher table to 'e' format

7cd0509

update output files

9aaddfe

gardner48 reviewed Jun 30, 2024

View reviewed changes

gardner48 and others added 5 commits July 3, 2024 16:49

Merge branch 'develop' into feature/real-format3

fbd7ad0

Merge branch 'develop' into feature/real-format3

77f6135

Remove sign padding from g format

aef939b

Merge branch 'develop' into feature/real-format3

50d8d8c

Merge branch 'develop' into feature/real-format3

9ec1fad

balos1 added this to the SUNDIALS Next milestone Sep 10, 2024

Steven-Roberts added 2 commits September 16, 2024 19:14

Merge branch 'develop' into feature/real-format3

e7d6397

Revert out files to make future merges easier

5399b9e

Steven-Roberts mentioned this pull request Sep 18, 2024

Feature/operator splitting #572

Merged

balos1 modified the milestones: SUNDIALS Next, SUNDIALS Next^2 Oct 1, 2024

Steven-Roberts added 12 commits October 17, 2024 14:19

Merge branch 'develop' into feature/real-format3

9bfb997

Merge branch 'develop' into feature/real-format3

629611b

Remove RSYM from LSRKStep

9067dd7

Finish updating print all stats functions

fe5a7e3

Merge branch 'develop' into feature/real-format3

96b9a24

Remove RSYM and old style PrintAllStats from newly merged code

813c630

Merge branch 'develop' into feature/real-format3

307cd5a

Remove RSYM macro after merge

dea1b28

Add todo comment

5d127d7

Merge branch 'develop' into feature/real-format3

303fa90

Remove sunsnprintf

819a649

Update logging out files

2725918

Apply formatter

f38cb88

drreynolds reviewed Dec 18, 2024

View reviewed changes

gardner48 reviewed Dec 18, 2024

View reviewed changes

Steven-Roberts and others added 3 commits December 18, 2024 07:13

Update include/sundials/sundials_types.h

dc34825

Co-authored-by: David Gardner <[email protected]>

Add signed format macro

77092bf

Add developer docs on format macros and print utilities

bfddd84

gardner48 requested changes Dec 19, 2024

View reviewed changes

Steven-Roberts and others added 4 commits December 19, 2024 09:25

Apply suggestions from code review

c864eea

Co-authored-by: David Gardner <[email protected]>

Merge branch 'develop' into feature/real-format3

096125e

Merge branch 'develop' into feature/real-format3

9578fc0

Merge branch 'develop' into feature/real-format3

250beed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add macro for consistent sunrealtype formatting #517

Add macro for consistent sunrealtype formatting #517

Steven-Roberts commented Jun 21, 2024

gardner48 Jun 28, 2024

gardner48 Jun 30, 2024

Steven-Roberts Jul 1, 2024

Steven-Roberts commented Dec 17, 2024

drreynolds left a comment

drreynolds Dec 18, 2024

drreynolds Dec 18, 2024

Steven-Roberts Dec 18, 2024

gardner48 Dec 18, 2024

drreynolds Dec 18, 2024

Steven-Roberts Dec 18, 2024 •

edited

Loading

gardner48 Dec 18, 2024

gardner48 commented Dec 18, 2024 •

edited

Loading

gardner48 left a comment

		%ignore SUN_FORMAT_E;
		%ignore SUN_FORMAT_G;

Add macro for consistent sunrealtype formatting #517

Are you sure you want to change the base?

Add macro for consistent sunrealtype formatting #517

Conversation

Steven-Roberts commented Jun 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Steven-Roberts commented Dec 17, 2024

drreynolds 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

Choose a reason for hiding this comment

Steven-Roberts Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gardner48 commented Dec 18, 2024 • edited Loading

gardner48 left a comment

Choose a reason for hiding this comment

Steven-Roberts Dec 18, 2024 •

edited

Loading

gardner48 commented Dec 18, 2024 •

edited

Loading