-
Notifications
You must be signed in to change notification settings - Fork 2
Add tool to upgrade collisions/closure in Hermes-3 #137
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
base: master
Are you sure you want to change the base?
Conversation
|
Should I go ahead and merge this or is there anything else we're waiting for? |
|
I guess we might just want to wait until the corresponding changes go into Hermes, in case any names change? |
|
The changes have been merged into Hermes, so I guess it makes sense to merge this now. |
|
The PR was merged prematurely. Let me finish my tests first |
|
Is there no way to just replace strings instead of clobbering all the formatting? It makes things a bit painful, especially for reactions: |
|
This builds on the more general purpose tool, which is based on parsing the input file. This means we can move inputs between sections, for instance. We're sacrificing the ability to preserve the formatting for correctness. It might be possible to get it to only apply the changes to areas touched by fixes, but I'm not sure how much effort that would be. |
|
I think that would be quite challenging. The "normalisation" of the file (to use its own terminology) seems to be a result of having parsed the input file into Python classes, which are then written back out using standardised formatting. |
|
The script does a confusing thing. It adds I think this is confusing because at first glance the top level components look unchanged, making it more challenging to spot that component names have now changed, and resulting in an input file mixing new and old component names. The old component names are in the same place as before as well. I would much prefer to make sure no old component name persists, and that the new components are all in the top level. |
|
The reason I took this approach was because the name of the component in the The reason |
|
Running a simulation with an input file generated by the converter fails - the initialisation is in a loop like the below. I have tested it on @oparry-ukaea's recent test (https://github.com/boutproject/hermes-3/blob/1a2f9e303e38b956425d3e43d4893f736065aa2c/tests/integrated/1D-recycling-dthe/data/BOUT.inp) which includes all of the new components apart from electron viscosity. I'm not sure what could have caused this... |
|
Ok, the print loop is actually a bug with @cmacmackin I can see that the way the upgrader deals with the new component names is more straightforward to implement, but I'm concerned that it will cause a lot of confusion for users, most of whom have never touched the component lists and don't have an in-depth understanding of the system. Would it be a lot of work to change the upgrader to rename the old components and keep everything top level? I also noticed that the upgrader doesn't ask for which kind of upgrade to perform. Is the intent for the same upgrader to be used for any further changes and keep it called "hermes"? |
The way the BOUT++ upgraders handle this is to specify which version you are upgrading from/to. We could do that. It really comes down to how many layers of subcommands we want. |
It's probably doable, it's just harder to cover every edge-case. |
|
@mikekryjak @ZedThree I've implemented Mike's requested changes. I guess this is probably ready to be merged now. |
|
Thanks Chris! Let me have a play early next week before it gets merged. |
|
@mikekryjak Have you had a chance to play with this at all yet? It would be good to get it merged, seeing as these changes to Hermes-3 are now in |
|
Sorry, still on leave. Will do next week
…On Fri, 2 Jan 2026, 11:21 Chris MacMackin, ***@***.***> wrote:
*cmacmackin* left a comment (boutproject/boutdata#137)
<#137 (comment)>
@mikekryjak <https://github.com/mikekryjak> Have you had a chance to play
with this at all yet? It would be good to get it merged, seeing as these
changes to Hermes-3 are now in master.
—
Reply to this email directly, view it on GitHub
<#137 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AO7DNNRWKAZ52TOD5W7FES34EZBAZAVCNFSM6AAAAACNVUCTFSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTOMBUHE4TMMRVHE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@cmacmackin thanks for the changes. I ran this on a test case and found that the script doesn't handle an existing block like this: Instead of changing the header to [braginskii_collisions], it appends this code to the end: This leaves |
boutproject/hermes-3#399 changes the names of a number of components and also split some up. This PR provides a tool for automatically upgrading BOUT.inp files to handle these breaking changes. To do this, I slightly altered the API for the existing scripts that update input files, to make them more generic.