-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove standard topmodel output when running from ngen #41
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spurious comment
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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