(Semi)-Compiled Inputs v2 - #1396
Conversation
…as rummy format and bypass the other input file reader. Includes a fix for Get for length 1 vectors that were read not as vectors
… Store the rummy deck in parameterinput now
|
Looks like I have some cmake issues in the library, but the parthenon parts should be good to review |
…sing native parser. Make types local as well
… Reverts parameter_input to very close to it's original state. Update rummy tests
|
I've refactored this to follow #1386 and put the new parser stuff separate from parameter_input |
lroberts36
left a comment
lroberts36
left a comment
There was a problem hiding this comment.
LGTM, thanks for separating it out from ParameterInput
| } else { | ||
| // Otherwise store as UnresolvedString to preserve full precision | ||
| return UnresolvedString(card.GetString(std::numeric_limits<double>::max_digits10)); | ||
| } |
There was a problem hiding this comment.
Why not explicitly type numeric values?
There was a problem hiding this comment.
At this stage I don't know whether it's an int or a double. Rummy is duck typed, so there is no distinction between int/double. So I'm not taking a side here and letting the user determine the type when they call GetOrAdd
There was a problem hiding this comment.
Should we be adding a vector of UnresolvedString as an option?
There was a problem hiding this comment.
The rummy "cards" are not vector aware. Adding the vector variants is done below near line 122 of this fiel.
There was a problem hiding this comment.
Yeah, I was wondering if it made more sense to store that as a vector of UnresolvedStrings rather than rebuilding a comma delimited string.
bf07773 to
7c5b1be
Compare
|
@adamdempsey90 a MR conflict in CMakelists. @pdmullen gentle reminder to review |
Fixed. This and #1418 should probably be merged together since I'm really only testing the combo in artemis right now. |
4 participants
PR Summary
This is a redo of the Rummy parsing integration and replaces #1336.
This is now based on the new parameter machinery in #1385.
Compared to #1336, this should be a backwards compatible change. The Rummy input files are automatically detected based on the syntax in the file (but that can be overridden). I have added a unit test and copied the
sparse_advectionregression test to a new one that changes the input file to use Rummy syntax. I've also added a documentation page describing what is allowed now.I can't seem to run most of the regression tests by hand (they timeout), so hopefully the CI does it correctly.
This should probably be merged with a squash.
This was done in collaboration with LLMs (but not claude code or codex).
PR Checklist