Remove standard topmodel output when running from ngen #41
Conversation
…xing accidental comment associated w/this file
…dated comments relate dto functions
…s of the new functions
…ratable parameters
…type input variables.
…for printing out updates relevant to calibratable parameters.
…y+num_time_delay_histo_ords, instead of just num_time_dealy_histo_ords. Also edited a print statement, and an error to provide assistance to user about how to fix error.
…e Q differently depending on if stand alon emode or not. Seeing discrepancies between master stand_alone results and current stand_alone results. Differences exist before these edits.
…ly to output when in stand_alone mode
…nd alone mode (i.e., stand_alone == 1)
…nted when in debug mode
|
tagging issue #32 |
…put-fromNGEN-resynced
There was a problem hiding this comment.
I'm generally curious if it isn't possible to simply do something like
if( stand_alone ) model->yes_print_output = FALSE;so that the existing state/variable can be used to disable the outputs without introducing a new flag/arg to several new functions that seems to be used in conjunction with yes_print_output.
| @@ -527,7 +521,7 @@ static int Finalize (Bmi *self) | |||
| free(model->dist_from_outlet); | |||
|
|
|||
| // Close output files only if opened in first place | |||
| if(model->yes_print_output == TRUE){ | |||
| if(model->yes_print_output == TRUE && model->stand_alone == TRUE){ // && model->stand_alone == TRUE){ | |||
| @@ -421,8 +421,7 @@ d_alloc(Qobs,(*nstep)); | |||
| //NJF This is dangerous, there is no validation between nstep and | |||
| //number of historgram ordinates, which is used to iterate Q in later steps | |||
| //so this could easily overflow if nstep is smaller than historgram ords | |||
| d_alloc(Q,(*nstep)); //NJF TODO validate that this works correctly | |||
| //When used in "stand alone" mode | |||
| d_alloc(Q,(*nstep)); | |||
There was a problem hiding this comment.
Was this ever validated? I don't believe I went through that code path in detail throughout the dyamic alloc refactoring. Might want to leave this comment until we are sure there isn't possible issues here in stand alone mode?
There was a problem hiding this comment.
I am able to run in stand alone mode and with NGEN. Output files topmod.out and hyd.out are produced and seem correct. Not sure if further validation is needed.
On a tangential note, to run in stand_alone I am using:
#!/bin/bash
# -lm links math library
gcc ./extern/topmodel/topmodel/src/main.c \
./extern/topmodel/topmodel/src/bmi_topmodel.c \
./extern/topmodel/topmodel/src/topmodel.c -o\
run_topmodel_bmi -lm
./run_topmodel_bmi
When I view the info in docs/STAND_ALONE.md, I do not see these instructions anywhere. If this is the expected way to execute in stand_alone, it may be worth adding those instructions there.
If it is not the expected way to execute in stand_alone mode, feel free to let me know!
There was a problem hiding this comment.
@Ben-Choat - The install doc gives instructions on standalone via cmake. It probably wouldn't be a bad idea to add a link in the STAND_ALONE.md. Thanks for pointing this out.
|
In terms of dynamic allocation validation, the unit tests are built with AddressSanitizer, but neither the standalone nor the ngen integration tests are. |
There was a problem hiding this comment.
This looks okay to me. Note that it addresses/closes issue #32.
|
@Ben-Choat - okay to merge? |
|
@madMatchstick , Yup, as far as I'm concerned it is good to merge. I'm not sure if it woud be better to squash or rebase, so I was hestitant to merge myself. |
[Short description explaining the high-level reason for the pull request]
Additions
Removals
Changes
Testing
Screenshots
Notes
Todos
Checklist
Testing checklist
Target Environment support
Accessibility
Other