Add option to convert cooling times in output file to seconds#148
Add option to convert cooling times in output file to seconds#148anu1217 wants to merge 31 commits intosvalinn:mainfrom
Conversation
|
There is a merge conflict to be resolved here thanks to #215 |
|
I've pulled from this branch and tested out these changes, and they appear to be working as expected. Some documentation changes are likely still needed. |
gonuke
left a comment
There was a problem hiding this comment.
I'm a little surprised that this compiles, given some mismatches in the header and code.
src/OutputFormat.C
Outdated
There was a problem hiding this comment.
I think we need to write the cooling time units for these ones, too, probably
There was a problem hiding this comment.
Should print out the units for the other responses now
85748c1 to
81d5d7f
Compare
Co-authored-by: Paul Wilson <paul.wilson@wisc.edu>
Co-authored-by: Paul Wilson <paul.wilson@wisc.edu>
Co-authored-by: Paul Wilson <paul.wilson@wisc.edu>
|
Before marking this as ready for review, I changed my approach to this problem and pushed all my changes to a different branch, before merging the new branch into this one. I believe this didn't execute the way I expected and left some extraneous changes, which I think have all been resolved now. |
gonuke
left a comment
There was a problem hiding this comment.
Sorry - this review was 1/2 done a while ago and got lost...
| if (cooltime_units == COOLTIME_S) // print cooling time converted to seconds | ||
| { | ||
| double t_sec = convertTime(ptr->coolingTime, ptr->units); | ||
| sprintf(textBuf, "%9.3e s ", t_sec); | ||
| } | ||
| else // print cooling time in default units | ||
| { | ||
| sprintf(textBuf, "%7g %c ", ptr->coolingTime, ptr->units); | ||
| } |
There was a problem hiding this comment.
Maybe a short function
std::string coolingTimeStr(int coolTimeUnit)
That does this to ensure consistency across different places we do this (currently only two)
src/Result.C
Outdated
| metricMult = 1; | ||
| } | ||
| cooltime_units = cooltimeType; | ||
| } |
There was a problem hiding this comment.
I'm not sure this is right...
Don't we still want to make sure metricMult is set correctly in the default case? An don't we want to update cooltime_units regardless of the value of normType?
There was a problem hiding this comment.
I think I may have deleted too many lines after getting rid of a switch statement for cooltimeType...
src/OutputFormat.C
Outdated
| input >> token; | ||
| next->cooltimeUnits = new char[strlen(token)+2]; |
There was a problem hiding this comment.
For better or worse, this is not backwards compatible and will make old input files fail. We should discuss how important that is...
There was a problem hiding this comment.
This should be backwards compatible now
correctly set metric multiplier and cooltime units remove unused break address feedback
add std namespace avoid crossing initialization move initialization of pos declare pos in block remove extra space reinitialize cooltimeUnits so that there is no extra space initialize cooltimeUnits with the length of the units token account for nonexisting cooling time units check condition with first character of token
Address feedback and test some changes
anu1217
left a comment
There was a problem hiding this comment.
I've addressed most of the feedback here and have tested these changes in a container.
src/Result.C
Outdated
| metricMult = 1; | ||
| } | ||
| cooltime_units = cooltimeType; | ||
| } |
There was a problem hiding this comment.
I think I may have deleted too many lines after getting rid of a switch statement for cooltimeType...
src/OutputFormat.C
Outdated
| input >> token; | ||
| next->cooltimeUnits = new char[strlen(token)+2]; |
There was a problem hiding this comment.
This should be backwards compatible now
gonuke
left a comment
There was a problem hiding this comment.
Sorry for the delay - I had some pending changes that I forgot to submit.
| switch (1<<type) | ||
| { | ||
| case OUTFMT_UNITS: | ||
| { |
| break; | ||
|
|
||
| delete[] next->cooltimeUnits; | ||
| next->cooltimeUnits = new char[strlen(token)+1]; |
There was a problem hiding this comment.
I'm not sure this is the right token to use to determine the length?
| } | ||
| else if (tolower(token[0]) == 'd') | ||
| { | ||
| input.seekg(pos); |
There was a problem hiding this comment.
What is this doing? Can't you just do nothing in this case?
| break; | ||
|
|
||
| delete[] next->cooltimeUnits; | ||
| next->cooltimeUnits = new char[strlen(token)+1]; |
There was a problem hiding this comment.
For better or worse, this is not backwards compatible and will make old input files fail. We should discuss how important that is...
There was a problem hiding this comment.
Perhaps we figure out a way to make it optional?
| if (input >> token) | ||
| { | ||
| if (tolower(token[0]) == 's') | ||
| { | ||
| next->cooltimeType = COOLTIME_S; | ||
|
|
||
| delete[] next->cooltimeUnits; | ||
| next->cooltimeUnits = new char[strlen(token)+1]; | ||
| strcpy(next->cooltimeUnits, token); | ||
| } | ||
| else if (tolower(token[0]) == 'd') | ||
| { | ||
| input.seekg(pos); | ||
| } | ||
| else | ||
| { | ||
| error(230, "Unknown cooling time unit '%s'", token); | ||
| } | ||
| } |
There was a problem hiding this comment.
Why might need to implement clang-formatting next, but this style is preferred:
| if (input >> token) | |
| { | |
| if (tolower(token[0]) == 's') | |
| { | |
| next->cooltimeType = COOLTIME_S; | |
| delete[] next->cooltimeUnits; | |
| next->cooltimeUnits = new char[strlen(token)+1]; | |
| strcpy(next->cooltimeUnits, token); | |
| } | |
| else if (tolower(token[0]) == 'd') | |
| { | |
| input.seekg(pos); | |
| } | |
| else | |
| { | |
| error(230, "Unknown cooling time unit '%s'", token); | |
| } | |
| } | |
| if (input >> token) { | |
| if (tolower(token[0]) == 's') { | |
| next->cooltimeType = COOLTIME_S; | |
| delete[] next->cooltimeUnits; | |
| next->cooltimeUnits = new char[strlen(token)+1]; | |
| strcpy(next->cooltimeUnits, token); | |
| } else if (tolower(token[0]) == 'd') { | |
| input.seekg(pos); | |
| } else { | |
| error(230, "Unknown cooling time unit '%s'", token); | |
| } | |
| } |
| switch (normType) | ||
| { |
There was a problem hiding this comment.
| switch (normType) | |
| { | |
| switch (normType) { |
This PR introduces changes that allow the user to select the units (default or seconds) in which cooling times are printed in the headers of the output tables, if the
unitskeyword is added in theoutputsection of the input file.Adding this in as a draft for now as this change has had many moving pieces, with changes to function definitions in multiple files, and I anticipate more work being necessary for everything to work as intended.
fixes #137