#1796: Include IllinoisGRMHD into the Toolkit, as an Arrangement ------------------------------------+--------------------------------------- Reporter: zachetie@… | Owner: Zachariah Etienne Type: enhancement | Status: new Priority: major | Milestone: ET_2015_11 Component: EinsteinToolkit thorn | Version: development version Resolution: | Keywords: GRMHD IllinoisGRMHD ------------------------------------+---------------------------------------
Comment (by Zach Etienne):
Hi Frank,
convert_to_HydroBase:
* Most thorns usually have the convention of starting with an upper case letter. Not a must, but these stand out a little because of that. Not a must change of course.
I renamed convert_to_HydroBase -> Convert_to_HydroBase
* It would be nice to provide a README file. It doesn't need to be long, just look at some examples from other thorns. Not a must, but a would be nice.
I added both README and LICENSE (BSD 2-clause) files.
* Why do you explicitly enable storage for ADMBase and HydroBase variables? Did you find that these thorns do this insufficiently?
A force of habit, as I really hate segfaults when these storage things
aren't set properly. I'm worried I might break something if I adjust this.
* You might want to schedule "in INITIAL after HydroBase_Initial", instead of in POSTINITIAL after SetTmunu. SetTmunu is, as far as I can see, also scheduled in Initial, and not in PostInitial, so this 'after' statement is always fulfilled already.
Scheduling in ET is a nightmare. As usual, this scheduling was setup
after a long period of trial and error. I'm worried I might break something if I adjust this.
* the make.code.defn file contains "Main make.code.defn file for thorn smallbPoyn" Why do you have it depend on CarpetRegrid2?
Fixed.
ID_converter_ILGRMHD:
* It would be nice to provide a README file. It doesn't need to be long, just look at some examples from other thorns. Not a must, but a would be nice.
I added both README and LICENSE (BSD 2-clause) files.
* There is a "shares: driver" statement without actually anything sharing.
Removed.
* Same question about explicit storage of other thorn's variables
Same response; worried I might break something.
* You schedule convertion IN HydroBase_Initial, instead of after it, explicitly depending on AFTER statements of a few known ID thorns. This is likely to fail for other ID thorns. Would it be possible to schedule AFTER HydroBase_Initial?
Yes, this is not desirable behavior. Still needs to be fixed.
* make.code.defn file for thorn IllinoisGRMHD_InitialData?
Fixed.
* The random perturbation code would be better in a separate thorn. Wouldn't this be also useful for other purposes than just when using the conversion?
It's conceivable it might be useful for other purposes, though the one
purpose I have in mind is code validation: By adding a perturbation in the initial data of order machine epsilon in a trusted version of IllinoisGRMHD, I can quickly measure how roundoff errors grow. This enables me to verify that new versions of IllinoisGRMHD agree to roundoff error with trusted versions. So I'd prefer to leave this be for the time being.
both:
* There are no testsuites
As IllinoisGRMHD is nearly useless without these compatibility-layer
thorns, I was waiting to add the testsuites to IllinoisGRMHD once these thorns have been added to the Toolkit.
* There is no (at least short) documentation. It doesn't need to be much, given these thorns.
Given how little these thorns actually do, I added a couple paragraphs
in the README files to document the thorns.