Refactor init() to smaller functions #34

Ben-Choat · 2023-08-16T17:32:17Z

Refactored init() within topmodel.c to four smaller functions. Ultimately, this was done to allow calibratable parameters to update secondary variables withib bmi_topmodel.c. For now, only topmodel.c has been edited.

Additions

four functions
convert_dist_to_histords(double *dist_from_outlet, int num_channels,
double chv, double rv, double dt, double tch[11]))
calc_time_delay_histogram(int max_time_delay_ordinates, int num_channels, double area,
double tch[11], int *num_time_delay_histo_ords, int *num_delay,
double **time_delay_histogram, double *cum_dist_area_with_dist)
init_water_balance(double **deficit_local, int max_atb_increments,
int num_topodex_values, double dt, double *sr0,
double *szm, double *Q0, double *t0, double tl,
double **stor_unsat_zone, double *szq,
double **deficit_root_zone, double *sbar,
double *bal)
init_discharge_array(int *num_delay, double *Q, double *Q0, double area,
int *num_time_delay_histo_ords, double **time_delay_histogram)

Removals

Changes

Moved portions of init() to four smaller functions, and init() now calls those four smaller functions.

Testing

Built and ran topmodel before refactoring and after refactoring and output was identical.
Edited and tested in Ubuntu 22.04

Screenshots

Notes

deficit_local is initialized, but not used within init(). Should it still appear there?

Todos

Edit bmi_topmodel.c to enable secondary variables to be calcualted from calibratable parameters defined within realization file.

Checklist

[x ] PR has an informative and human-readable title
[ x] Changes are limited to a single goal (no scope creep)
Code can be automatically merged (no conflicts)
Passes all existing automated tests
[ x] Any change in functionality is tested
[ x] New functions are documented (with a description, list of inputs, and expected output)
[ x] Visually tested in supported browsers and devices (see checklist below 👇)
Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)
[x ] Reviewers requested with the Reviewers tool

Target Environment support

[x ] Windows
[ x] Linux

Accessibility

[ x] Keyboard friendly
[ x] Screen reader friendly

Other

[ x] Is useable without CSS
[ x] Is useable without JS
[ x] No linting errors or warnings

…xing accidental comment associated w/this file

hellkite500 · 2023-08-16T21:46:19Z

include/topmodel_backup.h

Remove from this PR, the git history will contain the original file if you are concerned about loosing the previous implementation.

Will do. Will focus on removing extraneous changes/files from PR, better documenting within the code, and giving special attention to assumption around memory allocation. If this is not already done somewhere in the original code I may need to touch base with you on that sometime next week to make sure I'm going about it properly. Thanks for the feedback!

hellkite500 · 2023-08-16T21:46:43Z

src/WorkingFiles/bmi_topmodel_backup.c

Remove from the PR, the original is in the version history

hellkite500 · 2023-08-16T21:46:58Z

src/WorkingFiles/topmodel_backup.c

Remove from PR

hellkite500 · 2023-08-16T21:48:20Z

src/WorkingFiles/topmodel_init_refactored.c

Seems like maybe you ended up staging some working files you didn't intend to. I see now that a couple of these exist in src/WorkingFiles. You can always add to your local .gitignore to prevent spurious working directories from getting staged.

hellkite500 · 2023-08-16T22:08:32Z

src/topmodel.c

+ * **********************************/
+
+
+// FUNCTION TO CONVERT DISTANCE/AREA FORM TO TIME DELAY HISTOGRAM ORDINATES


Definitely reccomend documenting function paramers inline. using doxygen style comments can also help auto generate human readable docs in the future. Consider a comment like this

/** * Function to convert Distance/Area from T0 time delay historgram ordinates. * * <additional details, also known as long description, here e.g.> * This function converts paramters to m/time step DT with exception of XK0 * which must stay in m/h. * Q0 is already in m/time step * T0 is input as Ln(To) * * @param[in] dist_from_outlet pointer to array of length num_channels containing WHAT * @param[in] num_channels the number of channels, corresponds to the length of dist_from_outlet and tch arrays * @param[in] chv <is what???> * @param[in] rv <is what???> * @param[in] dt delta time, or time step, used to convert chv and rv * @param[out] tch <what is tch, generated by this function> */

I'll also note that I don't see an XK0 or T0 parameter anywhere in this function, so perhaps that comment needs to be dropped???

hellkite500 · 2023-08-16T22:16:48Z

src/topmodel.c

+
+    for(ir=1;ir<=(*num_time_delay_histo_ords);ir++)
+      {
+      time=(double)(*num_delay)+(double)ir;


again, this cast is suspicious, why does time need to be a double? both num_delay and ir are ints, so should time simply be an int as well?

Also from original code.

topmodel/refs/original_code_c/tmod9502.c

Line 733 in 9452b7b

time=(double)(*num_delay)+(double)ir;

@hellkite500, can you make heads or tails of this casting? The old Fortran code does it, too. Maybe a legacy of it being over 40 years old?

The only thing I can think of is that time was expected to be large, and a double type was used to avoid overflowing an integer with large enough time values? For signed (positive/negative) ints, an 8 bit int can hold numbers up to 127, a 16 bit int 32,767, 32-bit int 2,147,483,647, and a 64-bit int 9,223,372,036,854,775,807.

It is possible that original code was developed on something with only 16 bit integers and double was used to increase the largest possible time representation.

If we really want to ensure we can hold large representations for time, we can change the type of time to int64_t and use

#include <stdint.h>

to have a portable, compliant 64 bit int to put that into...

We can also use unsigned types if we know the values are not ever going to be negative.

The other point I'll make here is that even if time does need to be a floating point, you don't have to cast the int, you can safely add ints and store them in a double, the compiler will handle that just fine.

hellkite500 · 2023-08-16T22:23:35Z

src/topmodel.c

+    double time, a1, sumar, a2;
+    int j, ir;
+
+    if((*time_delay_histogram)==NULL)


If this function's responsibility is to calc the histogram, it probably should also be responsible for initializing memory. At the same time, its probably a good practice for any of these functions that accept pointers as INPUT to check the pointer is valid (e.g. not NULL) and simply throw an error if it is.

assert( time_delay_historgram != NULL ) assert( *time_delay_histogram != NULL ) assert( num_time_delay_histo_ords != NULL ) assert( num_delay != NULL ) assert( cum_dist_area_with_dist != NULL )

hellkite500 · 2023-08-16T22:27:11Z

src/topmodel.c

+// updates values associated with model structure
+// outputs used in TOPMOD() include num_time_delay_histo_ords, time_delay_histogram,
+// num_delay, and Q
+extern void calc_time_delay_histogram(int max_time_delay_ordinates, int num_channels, double area,


A common C style guide/implementation is to put all input params (things that are NOT modified by the function) first in the arg list, and all out or in/out params (things that will be modified by the function) last. I strongly recommend this!

Also its imperative in C codes to document assumptions on memory already being allocated vs the memory to be allocated and returned by the function. As noted below, this function could NOT check and allocate the time_delay_histogram, which is valid, but it should be documented in the doc string (see above comment on doxygen style doc strings) either way. Keeping the allocation his is OK, as long as the caller knows it was allocated in this function and can potentially deallocate it later.

hellkite500 · 2023-08-16T22:28:48Z

src/topmodel.c

+// updates values associated with model structure
+// outputs used in TOPMOD() include sbar, bal, deficit_root_zone
+
+extern void init_water_balance(double **deficit_local, int max_atb_increments, 


another good example of ordering params and documenting behavior. Most "init" functions are safe to assume are doing some allocation/initialization, but being clear about WHICH variables are being allocated is important

hellkite500 · 2023-08-16T22:29:45Z

src/topmodel.c

+			)
+{
+    // declare local variables
+    double sum;


can declare/init sum in one line for berevity

double sum = 0.0;

hellkite500 · 2023-08-16T22:33:48Z

Left some initial review comments, and mashed the button before I could elaborate on my requested changes. Definitely need to address some of the array declaration/passing with static sizes (11) and remove the unnecessary files. The rest of the comments are stylistic and documentation related, but would likely be worth addressing to set the tone for future refactoring and/or contributions/maintainers -- they will thank us later!

SnowHydrology

@Ben-Choat, please focus on making the changes suggested by @hellkite500, particularly the removal of all temporary files (as he noted, GitHub does the version tracking—no need for WorkingFiles or backup) and the addition of comments reflecting the newly implemented order of parameters.

You can also suggest @ajkhattak and @madMatchstick as reviewers while I'm out. Thanks!

SnowHydrology · 2023-08-18T13:55:37Z

src/topmodel.c

+// FUNCTION TO CONVERT DISTANCE/AREA FORM TO TIME DELAY HISTOGRAM ORDINATES
+// outputs used in subsequent functions include tch
+extern void convert_dist_to_histords(double *dist_from_outlet, int num_channels,
+					double chv, double rv, double dt, double tch[11])


@hellkite500, my reading of the original code is that TCH(10) is a hard-coded limit corresponding to the number of channels, a parameter value (num_channels) read in from subcat.dat.

This does not seem like the correct way of implementing things because there is no guardrail, as far as I know, preventing num_channels from being > 10. How would you recommend handling this?

Also, as a C dilettante, why would it be TCH(10) in Fortran and tch[11] in C (the 10 vs. 11 part, not the rest of the syntax)?

SnowHydrology · 2023-08-18T13:59:21Z

src/topmodel.c

+
+    for(ir=1;ir<=(*num_time_delay_histo_ords);ir++)
+      {
+      time=(double)(*num_delay)+(double)ir;


@hellkite500, can you make heads or tails of this casting? The old Fortran code does it, too. Maybe a legacy of it being over 40 years old?

madMatchstick · 2023-08-22T19:45:47Z

@Ben-Choat - Looks good overall

atm gh actions/workflows check standalone and unit test scripts, all of which passed.

Can you please add under "testing" a single bullet detailing ngen build/run results & perhaps paste any console out if appropriate. Even a comment stating "tested updated code in ngen without error, results seem reasonable" would suffice.
Best to reference local machine/OS somewhere in PR for good measure (unless I missed it) :)

-thx!

Has been addressed.

madMatchstick

Updates run in ngen coupled with PET, as outlined in wiki. Output seems reasonable.

Ben-Choat added 11 commits July 25, 2023 13:54

testing fork

11b32be

removing test file

bfea366

Failed attempt at implementing calibratable params

fe7cd9b

backup/scratch working files to be removed later

bdc39b0

fixing an accidental comment included with this file

4d40b80

remove nstep dt from config and set to 1 in else block initalize - fi…

9c0745c

…xing accidental comment associated w/this file

refactoring init to smaller functions

122ae65

refactored init() into smaller functions

e29eed6

refactored init() into smaller functions

d8c7221

refactored init() to smaller functions

bbfa55c

refactoring init() to smaller functions - update comments

60dbc77

Ben-Choat requested review from hellkite500 and madMatchstick August 16, 2023 17:32

refactored init() to smaller functions - added missing \+

53b0419

hellkite500 previously requested changes Aug 16, 2023

View reviewed changes

SnowHydrology previously requested changes Aug 18, 2023

View reviewed changes

Ben-Choat added 6 commits August 24, 2023 13:39

removing folder that should have not been included in last commit

455281c

updated comments for clarity related to refactored init() functions

c94907e

removing unneeded file that was not intended to be included

6ae4ec0

added new function prototypes to topmodel.h

1d519f2

rearranged inputs to init() so inputs come first and outputs come last

a7d8bc6

updated some commenting

7a71d66

Ben-Choat marked this pull request as ready for review September 8, 2023 21:19

madMatchstick approved these changes Dec 1, 2023

View reviewed changes

madMatchstick merged commit 4995885 into NOAA-OWP:master Dec 1, 2023
2 checks passed

madMatchstick mentioned this pull request Dec 1, 2023

Implementing calibratable parameters in tomodel, using bmi. #37

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor init() to smaller functions #34

Refactor init() to smaller functions #34

Ben-Choat commented Aug 16, 2023 •

edited

Loading

hellkite500 Aug 16, 2023

Ben-Choat Aug 17, 2023

hellkite500 Aug 16, 2023

hellkite500 Aug 16, 2023

hellkite500 Aug 16, 2023

hellkite500 Aug 16, 2023

hellkite500 Aug 16, 2023

Ben-Choat Aug 17, 2023

SnowHydrology Aug 18, 2023

hellkite500 Aug 18, 2023

hellkite500 Aug 16, 2023

hellkite500 Aug 16, 2023

hellkite500 Aug 16, 2023

hellkite500 Aug 16, 2023

hellkite500 commented Aug 16, 2023

SnowHydrology left a comment

SnowHydrology Aug 18, 2023

SnowHydrology Aug 18, 2023

madMatchstick commented Aug 22, 2023

madMatchstick left a comment

		* **********************************/


		// FUNCTION TO CONVERT DISTANCE/AREA FORM TO TIME DELAY HISTOGRAM ORDINATES

Refactor init() to smaller functions #34

Refactor init() to smaller functions #34

Conversation

Ben-Choat commented Aug 16, 2023 • edited Loading

Additions

Removals

Changes

Testing

Screenshots

Notes

Todos

Checklist

Target Environment support

Accessibility

Other

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

hellkite500 commented Aug 16, 2023

SnowHydrology left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madMatchstick commented Aug 22, 2023

madMatchstick left a comment

Choose a reason for hiding this comment

Ben-Choat commented Aug 16, 2023 •

edited

Loading