Change setting import model to use a placeholder value

[Andrew42]

This PR changes the way the Gridpack class sets the import model line of a process card. Previously, if the replace_model or use_coupling_model options weren't used, it would default to whatever was in the original process card and in the case that either of those options were specified it would look for the specific string dim6top_LO_UFO in order to do the replacement. If the process card's import model line didn't have that exact string, then both of these options would fail silently.

I've changed the search string that's to be replaced to be PLACEHOLDER_MODEL, which should make it much more obvious that it is simply a placeholder that gets replaced when code copies the MG cards around, This makes the handling of different process cards much more consistent. I've also updated all the existing process cards to now use import model PLACEHOLDER_MODEL.

The main downside of this approach is that this means you can no longer specify the MG model in the (template) process card directly. You have to do it via the replace_model option of the Gridpack class. I think this is acceptable since the entire point of the GridpackGeneration tools is to abstract away having to deal directly with editing and moving the MG cards when making a gridpack.

I'm of course open to suggestions about whether we think this is a good idea or not or if there's another approach to try instead. One more thing I didn't implement, but we might want to include in this PR is an explicit check that the template process card that's being parsed has the line "import model PLACEHOLDER_MODEL", so if a user adds their own new process card and didn't realize they should have this special token, the code can yell at them and tell them how to fix it.

[bryates]

Looks good to me. Thanks for making this change.

[kmohrman]

Hi @Andrew42, thanks for the interesting PR. I agree that it would be nice to make the string replacement a bit more standardized, but I am not sure that this is exactly the way we want to do it.

With this PR, it would mean that every process card would have to include PLACEHOLDER_MODEL, and the user would be required to use the string replacement option, right?

I am wondering if it might be more flexible and transparent to the user if the string replacement utility required the user to provide the exact string to be replaced (and what it should be replaced with). This would mean that the option would not be required (if the user just wants to use the model they've specified in the process card). And it would also mean that there would be no ambiguity about what is being replaced.

Wondering about your (and @bryates ) thoughts on this?

[bryates]

Hi @Andrew42, thanks for the interesting PR. I agree that it would be nice to make the string replacement a bit more standardized, but I am not sure that this is exactly the way we want to do it.

With this PR, it would mean that every process card would have to include PLACEHOLDER_MODEL, and the user would be required to use the string replacement option, right?

I am wondering if it might be more flexible and transparent to the user if the string replacement utility required the user to provide the exact string to be replaced (and what it should be replaced with). This would mean that the option would not be required (if the user just wants to use the model they've specified in the process card). And it would also mean that there would be no ambiguity about what is being replaced.

Wondering about your (and @bryates ) thoughts on this?

I like this suggestion. @Andrew42 do you think you'd have time to write this up? Essentially an extra argument to the python function (default None) that would tell it what string we would like to replace.

[Andrew42]

@kmohrman and @bryates I've reverted my initial changes and implemented the approach suggested by @kmohrman. In this implementation, the replace_model option must now be a list (or tuple) of length 2, where the first argument is the name of the old model and the second argument is the name of the new model that is to replace the old model.

[bryates]

I'm happy with this now. Once @kmohrman approves, we can merge.

[Andrew42]

Modified the README to be a little bit cleaner

[kmohrman]

Thanks @Andrew42. Looks good to me. I have not run it myself to test the new functionality (or even just e.g. make sure it does not crash). But I'm guessing you have :)
But just to be explicit for documentation purposes (especially since we don't have any CI), could you confirm that you've run it and it does not crash and it's working as expected?

If so I'd be happy to proceed with the merge.