add preparser to julia native #48
Conversation
|
Thanks for the draft!
If you mean as a pre-step before actually parsing the input data, but I'm not sure what the advantage is over parsing it directly in place apart from the availability of the intermediate data-structure. I don't think this strategy can be used for parsing in the end though, since AFAIU it is required to know the specific keywords to parse the file correctly. For example, in the file I linked to in the issue, So I think the more suitable option is to have a nested structure, and not a linear vector. data = Dict(
:parts => Dict{String, AbaqusPart}(p1 => AbaqusPart(), p2 => AbaqusPart()),
:assembly => AbaqusAssembly(), # Or maybe [Ordered]Dict{String, AbaqusInstance}
:materials => Dict{String, AbaqusMaterial}(),
:steps => OrderedDict{String, AbaqusStep}()
)Where the But as you say, this will be less flexible than simply parsing each key separately, but doing only that doesn't make that much sense to me in this package, since I don't think it gives much extra value wrt. how to use the data later. And doing this properly might be a quite big task, which in the end might turn out to lead to too much code for it to be suitable for inclusion here. I don't want to be too dismissive (cause I think if it can be done in a sleek fashion it would be very nice), but also I don't want you to spend hours on implementing only for the PR not getting accepted (since this would be a nice-to-have addon, but not the core functionality of the package). |
|
Thanks for the Feedback! I still think that it's usefull to first parse the .inp file directly into the julia native datastructure before interpreting the data. This parsing step doesn't remove any information, it just makes it easier to work with the data. I've implemented the I do agree that we should use a nested data structure if we want to properly support boundary conditions in the future. However, using this preparser should be a big step in the right direction, as it would make it much easer to construct this nested data as we then don't have to worry about parsing this data on the fly now. |
There was a problem hiding this comment.
Thanks for taking the time to write this up!
After understanding how you think, I do agree that this is more flexible, and seems in general like a nice idea!
One issue, however, is that it is almost 2x slower than the current implementation. I think this extra flexibility can motivate a slightly slower implementation, e.g. 20 % slower or so, but almost 2x seems a bit too much? I guess the reason is that the process is split into two parts. If the performance issues can be mitigated, I think this strategy would be beneficial!
Here is a benchmark file to test.
hex100x3.zip
| for (i, s) in pairs(quoted_split) | ||
| if isodd(i) | ||
| s = replace(s, " " => "") | ||
| s = uppercase(s) |
There was a problem hiding this comment.
Even if this is not taken into account for Abaqus, we should not enforce that all is upper-cased. This isn't nice for users since they keys in the sets don't match their expectations.
| struct AbaqusInp | ||
| blocks::Vector{AbaqusInpBlock} | ||
| end |
There was a problem hiding this comment.
Why do we need this wrapper?
Seems better to me to store as OrderedDict{String, AbaqusInpBlock} or Vector{Pair{String, AbaqusInpBlock}} to allow iterating over the keys.
| msg *= "Tip: If you want a single grid, merge parts and differentiated by Abaqus sets\n" | ||
| msg *= " If you want multiple grids, split into multiple input files" | ||
| throw(InvalidFileContent(msg)) | ||
| for inpblock in inpblocks(inp) |
There was a problem hiding this comment.
This would be nice if inpblocks(inp) was an ordered dict,
for (keyword, block) in ... would be nicer.
| line = readline(f) | ||
| while startswith(line, "**") | ||
| line = strip(readline(f)) | ||
| isodd(count('"', line)) && throw(InvalidFileContent("Quoted strings cannot span multiple lines!")) |
There was a problem hiding this comment.
I suppose a single quotation mark is allowed inside a comment so we shouldn't error here?
I've started working on #47. This is just a quickly thrown together draft but
read_keywordsshould correctly translate any valid .inp file (as defined here) into julia native datatypes while parsing the input as much as possible without any knowledge about the individual keyword specifications. This should make it easier to add support for more keywords in the future.Is that something thats going in the right direction or should I take a different approach?