Meudon_Bin_NS update

Issue #2039 closed
Frank Löffler created an issue

The attached patch generalizes the Meudon_Bin_NS thorn: - to be able to use any value for 'gamma' (EoS) other than 2 - to be able to use an EoS provided by EOSOmni. In particular, this enables piecewise polytropes of any kind, as long as the initial data was generated using the same "table"

... along with a few minor changes:

  • check that the input file name exists
  • add a few more parameter descriptions
  • allow setting the derivatives of the gauge to zero while still taking the gauge from Lorene

While a patch is attached, the new version could probably be tested the easiest from

https://bitbucket.org/GravityPR/prthorns/src/master/Meudon_Bin_NS

Keyword: parma

Comments (17)

  1. anonymous
    • changed status to open
    • removed comment

    I have a non-nitpicky one. Namely to return an error code to the OS if the file open test fails.

  2. Frank Löffler reporter
    • changed status to resolved
    • removed comment

    Since this was a trivial change, and the patch was already reviewed ok otherwise: applied.

  3. Roland Haas
    • removed comment

    Please don't do that. At least give us a day to check the trivial changes before you commit them. I agree I also commit without a full review but at least I announce that and give people some days to do review it after all.

    • what about the issue that right now there are two checks for the input files existing in the thorn. One that Frank added in patch and one that Roland had added a while ago (svn says 2012-01-26) as a patch to LORENE itself and that Meudon_Bin_NS uses via the try{...}catch{...} construct. Only one should be used I think. Which one I don't really mind. The one in this patch maybe easier to maintain since it does not require a change to LORENE while the change to LORENE is safer since it actually checks the LORENE could open the file rather than Cactus could (though I am not sure how one could work and the other fail)
    • it would be good to add a comment to the commit message explaining the implications of the failure of the previous Meudon_Bin_NS thorn to handle cases where Gamma!=2 in the atmosphere (that was the case where it could show up, was it?)
    • how is now handling the case of a hot eos (or generally whether keytemp is set to 0 or 1)?
  4. Frank Löffler reporter
    • removed comment

    At least give us a day to check the trivial changes before you commit them.

    Ok. All I did was replacing CCTK_VInfo with CCTK_VError.

    what about the issue that right now there are two checks for the input files existing in the thorn.

    What about it? I don't think checking twice is a problem. LORENE should contain a check, as it might be also used by other thorns than Meudon_Bin_NS. On the other hand, having a check in Meudon_Bin_NS allows for a better error message, since here you know which thorn actually caused the problem. This isn't critical for performance, and shouldn't fail in cases the others would not fail. Do you see a problem with checking twice?

    it would be good to add a comment to the commit message explaining

    I intend to do this in the release notes. The reason being that I don't think it would be visible enough in a commit message. I would now add something, but commit messages cannot be changed.

    how is now handling the case of a hot eos

    The explicit '4' as eos index was replaced by a check for storage of temperature and Y_e, and I call EOS_Omni_press accordingly. (I thought we discussed this change already. I'm sorry if I misremembered) There is a parameter to specify whether to take eps either from Lorene data, or to recalculate it, and depending on that key_temp is set to 0 or 1.

  5. Roland Haas
    • removed comment

    Ok. All I did was replacing CCTK_VInfo with CCTK_VError. That one is fine. :-)

    What about it? I don't think checking twice is a problem. LORENE should contain a check, as it might be also used by other thorns than Meudon_Bin_NS. On the other hand, having a check in Meudon_Bin_NS allows for a better error message, since here you know which thorn actually caused the problem. This isn't critical for performance, and shouldn't fail in cases the others would not fail. Do you see a problem with checking twice?

    There is a commit to Meudon_Bin_NS (64fc456 from 5 years ago) that goes along with the one in LORENE that does report the thorn (namely in the try{...}catch{...} block with the catch around line 245

      } catch (ios::failure e) {
        CCTK_VWarn (CCTK_WARN_ABORT, __LINE__, __FILE__, CCTK_THORNSTRING,
                    "Could not read initial data from file '%s': %s", filename, e.what());
      }
    

    So right now there are two parts of the code that do (almost) the same thing. I would want to keep only one of them to make maintenance easier. Otherwise depending on who looks at the code they may think that one or the other part of the code is actually the one that outputs errors when a file is not found (depending on whether they start reading the source code from the end or from the top of the file for example).

    it would be good to add a comment to the commit message explaining

    I intend to do this in the release notes. The reason being that I don't think it would be visible enough in a commit message. I would now add something, but commit messages cannot be changed. Release message is certainly a place to put it, I would just put it also into the commit message so that we don't have to rely in persons remembering all the release messages.

    how is now handling the case of a hot eos

    The explicit '4' as eos index was replaced by a check for storage of temperature and Y_e, and I call EOS_Omni_press accordingly. (I thought we discussed this change already. I'm sorry if I misremembered) There is a parameter to specify whether to take eps either from Lorene data, or to recalculate it, and depending on that key_temp is set to 0 or 1. Does that always work (and gives the very same results as before for the same correct set of parameter values)? I thought that before eg for a polytropic EOS it would pass havetemp=1 even though there was not storage for temperature?

  6. Frank Löffler reporter
    • removed comment

    You are right about the file existence check. I removed the new one, as the older covers more cases and includes the new check.

    Does that always work (and gives the very same results as before for the same correct set of parameter values)? I thought that before eg for a polytropic EOS it would pass havetemp=1 even though there was not storage for temperature?

    The thorn includes a new paramter recalculate_eps, which by default is 1. This will recalculate eps by passing '1' as keytemp to the EOS routines. If the EOS values are consistent, this should give the same results as if taken from Lorene directly. Calling the EOS routines makes sure that the hydro variables are consistent at least with respect to the evolution EOS.

    If you set the parameter to 0, eps will be used from Lorene, and '0' will be passed as keytemp to the EOS routines, causing them to not overwrite eps.

    Before this change, the EOS routines were not called at all, a polytrope was hard-coded, and eps taken from Lorene. In this sense, there is a change in the behavior when using a polytrope and for some reason eps from Lorene isn't the same as eps using the EOS routines. This should only make sense if you want to perturb the stars, which really doesn't make a lot of sense for Lorene Bin_NS data I would think. Even if you'd want that, this should be done in other ways than like this.

  7. Log in to comment