-
Notifications
You must be signed in to change notification settings - Fork 19
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
MonteCarloGenerate: Add more metadata to MonteCarlo_Meta_data_output #1608
Conversation
FYI @sharmeye @hchen99 @jmpenn I believe this is ready for review. I've got some intermediate commits on the branch, one of which was producing a segfault in |
Thanks, Dan! We'll review. |
trick_source/sim_services/MonteCarloGeneration/mc_variable_random_normal.cc
Outdated
Show resolved
Hide resolved
21df611
to
31db8c5
Compare
std::string MonteCarloVariable::summarize_variable() const | ||
{ | ||
std::ostringstream ss; | ||
ss << variable_name << std::string(": type=") << get_type_str() << std::string(","); |
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 there a reason you ended the line printout with a comma? The other summarize_variable functions don't have the comma at the end
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 think the comma should be removed here, then prepended to all derived class methods instead. In it's current state it results in an extra comma at the end if summarize_variable
isn't called polymorphically:
test.x_file_lookup[2]: type=Prescribed, max_skip=0, is_dependent=1, filename=Modified_data/datafile.txt, column_number=1, first_column_number=1
test.x_fixed_value_double: type=Constant,
I'll take a look at making that change here soon.
Thanks @drcurry2! By the way - am I correct to assume the key=<value>, key=<value>
format I've implemented here will work for your post processing tool use case? It's a slightly different format than your implementation but all the info is still there.
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.
The format works, I just need to make minor edits to the veras processing to read the file with your changes. Already testing it out
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.
@ddj116 Are you still going to make the changes you mentioned above ^
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.
Yes I believe I made those fixes already. Looks like the branch just merged so it's in Trick now. We'll have to update our reference to the Trick submodule before this goes live in ramtares. Prefer to wait for the next release but we can supersede that if needed, ping me if you still need this capability and it hasn't yet entered ramtares production branches.
@hchen99 @jmpenn @sharmeye I can't make any sense of the MacOS failure to build Trick - something about errors coming from |
I also checked out this branch and having no problem building trick and all tests on my Mac. I am gonna to give it a try in a container to see if anything needs updating for Mac workflow. Btw, thank y'all for addressing review comments. |
Ok, the latest github macos 12 runner image had an update recently that had python updated from 3.11 to 3.12. And looks like Python 3.12 has following legacy Unicode APIs removed and swig_int_templates.hh uses PyUnicode_GET_SIZE function that has been removed thus the error.
|
@ddj116 if you merge the latest master commit into this branch the MacOS test should pass. Go ahead and merge in the latest master commit and if it passes we'll merge this into the master branch. |
bf4115d
to
10d77d6
Compare
I merged with
Comparing the same build line to the last successful set of jobs, there's a difference in the value of |
Probably more related to the the -DLIBCLANG_MAJOR. The last success build was using -DLIBCLANG_MAJOR=15 and the failed one using -DLIBCLANG_MAJOR=16. Users have had issues with recent RHEL 8 update with llvm16. Trick llvm16 support is in work. |
@ddj116 try merging master in again now. Every time you try to get this pull brought in something like this happens. Are you sure this branch isn't cursed? |
* Add type, dispersion, min_value, max_value and other relevant internal members of MonteCarloVariable* classes to the output of MonteCarlo_Meta_data_output. Motivation is for users wanting to post-process dispersion parameters used during generation of runs * Protect against invalid memory access when length of values is zero in MonteCarloVariableRandomStringSet::generate_assignment(). Add a new verif sim warning case to cover these new lines * Update new verif data for SIM_mc_generation to support these changes Closes #1574
10d77d6
to
bbf48ba
Compare
Originally requested by a user in discussions in Ramtares GitLab merge request 6053, this MR implements a better and more robust solution than the originally proposed approach.
type
,dispersion
,min_value
,max_value
and other relevant internal members ofMonteCarloVariable*
classes to the output ofMonteCarlo_Meta_data_output
MonteCarloVariableRandomStringSet::generate_assignment()
if no string values are added to the instance. Add a verif sim run to cover this case.