Change Seed_Magnetic_Fields scheduling so it can be used by more thorns

[Samuel Cupp]

Right now, this thorn schedules based on the ID_converter_ILGRMHD function, preventing it from being used out-of-the-box by GRHayLMHD. I could change the scheduling to explicitly depend on this thorn, but a cleaner solution is to make their scheduling be based on HydroBase’s groups, providing a more consistent interface. As such, I propose that the seeding function be set to run before HydroBase_Prim2ConInitial. There’s a similar issue with Seed_Magnetic_Fields_BNS in that it can only be guaranteed to work with Meudon_Bin_NS.

Since both thorns should be changed to behave in a similar manner, I propose simply merging them into one. An example of this is in a PR. However, I’m not sure of the best way to handle the deprecation/behavior changes. It might be simpler to just make a new thorn (possibly in the GRHayL repo) that provides the combined thorn while keeping the old ones in their current state.

This PR also contains a bug fix for ID_converter_GiRaFFE and a very small optional feature for IllinoisGRMHD which I can make separately if we decide to make a new thorn instead of changing the existing one.

[Samuel Cupp]

@Zach Etienne , I would like your input on how you’d like me to change the scheduling, and whether you prefer two thorns or one. It seems strange for Seed_Magnetic_Fields_BNS to be in the diagnostics repo, especially when we can just have 1 thorn to provide both.

[Samuel Cupp]

@Roland Haas In regards to deprecation, correct me if I’m wrong: the best approach to merge the two thorns is to
A) Introduce the extra stuff to Seed_Magnetic_Fields (SMF) to allow for BNS as well
B) Ensure the default behavior of SMF is as before
C) Add deprecation warning for SMF_BNS thorn
D) In the release after the one where this is added, remove the SMF_BNS thorn entirely

[Samuel Cupp]

Also, what is the preferred method for alerting users to a deprecated feature? CCTK_VWARN?CCTK_VINFO?

[Roland Haas]

Hello Sam, what official docs we have on this is here:

https://docs.einsteintoolkit.org/et-docs/Policies_to_retire_functionality

So more or less what you describe.

Expect this to still catch people by surprise though. There is an (inofficial since I don't think we ever really discussed keeping it) list of deprecated and retired functionality that I try to keep here: https://docs.einsteintoolkit.org/et-docs/Deprecated_features (but the last entry is from 2020 so it is almost certainly not up to date).

No warning in the thorn to be deprecated is needed. Other codes do that, increasing levels of annoyance as the deprecation date, which is a fixed date recorded in the code, comes closer, up to and including refusing to run if the compiled functionality is too old. In the ET the warning comes in the form of a list of deprecated features in the release announcements and discussion in the ET calls.

Previous advise wrt to breaking changes is eg here:

Note that you cannot deprecate (change) default parameter values, since that would lead to silent changes in behaviour. This is not foolproof of course, just look at the trouble we have with GRHydro::sources_spatial_order and ADMMacros::spatial_order. Or at least it must have been a very very long time that anyone has ever relied on that default.

[Samuel Cupp]

Yeah, the only parameter changes are that the Afield_type parameter options are ambiguously named if I added in BNS as well, so I prepended TOV or BNS to all the keywords. This doesn’t matter for BNS, since those are new to the thorn. For the TOV case, I also have the old parameter names in there for now with a deprecation warning. The behavior is identical (all the cases have something like if(old_name || new_name). Eventually, I would want to phase out the old_name, but that could be done at any time, basically. The default is new_name, but there’s no real difference between the two.

[Samuel Cupp]

Merged into master and included as of the ET_2023_11 release.