[ET Trac] [Einstein Toolkit] #1796: Include IllinoisGRMHD into the Toolkit, as an Arrangement

[Einstein Toolkit]

#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.

-- 
Ticket URL: <https://trac.einsteintoolkit.org/ticket/1796#comment:8>
Einstein Toolkit <http://einsteintoolkit.org>
The Einstein Toolkit