Skip to content

Fixes for gmsh parser for periodic meshes#93

Merged
vikaskurapati merged 1 commit intomasterfrom
vikas/periodic
Apr 27, 2026
Merged

Fixes for gmsh parser for periodic meshes#93
vikaskurapati merged 1 commit intomasterfrom
vikas/periodic

Conversation

@vikaskurapati
Copy link
Copy Markdown
Contributor

Still draft as I am not sure if this fix goes well with the project, and I used AI to generate this code. @davschneller, do these changes give you a hint on why the dummy periodic mesh (uploaded in this comment) was getting stuck with PUMGen?

Contains AI generated code (GPT-Codex-5.3 as part of copilot CLI)
periodic.zip

Contains AI generated code (GPT-Codex-5.3 as part of copilot CLI)
@vikaskurapati vikaskurapati marked this pull request as draft April 27, 2026 12:18
@davschneller
Copy link
Copy Markdown
Contributor

davschneller commented Apr 27, 2026

Looking at it—it's exactly what the PR fixes: namely the Affine keyword before the affine transform seems to have tripped the parser. I would classify the token differently, however (as string signifies something in ", i.e. quotation marks). More like a generic keyword or the likes.
Consuming one token for unknown is fine IMO.

Though it seems like there is a chance you'll get only 12 entries, not 16? https://gmsh.info/doc/texinfo/gmsh.html#index-Affine-_007b-expression_002dlist-_007d-_007b-transform_002dlist-_007d (maybe I was wrong there)

Also the parser for GMSH in version 4 should be good before this PR already (at least that's what worked for me back then—but great to see that we can now also handle version 2).

@vikaskurapati
Copy link
Copy Markdown
Contributor Author

Looking at it—it's exactly what the PR fixes: namely the Affine keyword seems to have tripped the parser. I would classify the token differently, however (as string signifies something in ", i.e. quotation marks). More like a generic keyword or the likes. Consuming one token for unknown is fine IMO.

OK.

Though it seems like there is a chance you'll get only 12 entries, not 16? https://gmsh.info/doc/texinfo/gmsh.html#index-Affine-_007b-expression_002dlist-_007d-_007b-transform_002dlist-_007d (maybe I was wrong there)

I am unsure about it too.

Also the parser for GMSH in version 4 should be good before this PR already (at least that's what worked for me back then—but great to see that we can now also handle version 2).

Yes, I was using version 2 because most of my meshes are adapted from the older remnants, and I did not want to spend a lot of time modifying them all.

So do you think it is good enough to merge this, should I mark it ready for review? Or is it not worth having in the main branch, and I just use this only when necessary from this branch?

Copy link
Copy Markdown
Contributor

@davschneller davschneller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd merge it. We should support it IMO. :)

(same for eventually Netcdf etc.; not sure if Simmodeler supports it)

@vikaskurapati vikaskurapati marked this pull request as ready for review April 27, 2026 13:54
@vikaskurapati vikaskurapati merged commit 3924d82 into master Apr 27, 2026
3 checks passed
@vikaskurapati vikaskurapati deleted the vikas/periodic branch April 27, 2026 13:54
@vikaskurapati vikaskurapati restored the vikas/periodic branch April 29, 2026 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants