-
assigned issue to
inlcude FLRWSolver in ET
FLRWSolver provides cosmological initial conditions based on a Friedmann-Lemaitre-Robertson-Walker (FLRW) spacetime, with and without small perturbations.
It also contains a set of tools to create and analyze the data.
If can be found at: https://github.com/hayleyjm/FLRWSolver_public
Comments (47)
-
reporter -
reporter Reviewer: @helvi witek
-
Hi - I’m planning to use FLRWSolver in my research (I’m a PhD student at Queen Mary University of London working on testing GR/modified gravity in cosmology), so having FLRWSolver included in the ET would be really useful to me
-
Hi, I am Michele, a PhD student at the Center for Theoretical Physics in Warsaw and ET user. I believe that having the FLRWSolver thorn included in the ET would be extremely useful for all the users interested in cosmology. The thorn has already a place among the main codes for general-relativistic simulations of cosmological structure formation (see arXiv:2003.08014). I've used the FLRWSolver in my research on relativistic light propagation and observables extrapolation, see arXiv:2107.06306, and I plan to use the thorn in my future works.
-
Hi, I am a postdoc at the Max Planck Institute for gravitational Physics in Hannover. Before this I did my PhD on inhomogeneous cosmology and the impact of structure formation in the late Universe on the large-scale dynamics, and this is still part of my research interests. As such, I am currently using the ET and the FLRWSolver thorn for some simple cosmological setups and planning to use them in the near future for more involved cosmological simulations. I think that it would be very useful for such applications of the ET to have it include FLRWSolver in future releases.
-
I’m a PhD student from Argentina and I’m also interested in using FLRWSolver for future applications. I think it would be very useful if it’s included in the ETK.
-
I’m a PhD student at the University of Canterbury, New Zealand, working with Hayley Macpherson (the maintainer of the thorn) and simulations using FLRWSolver are a major component of my PhD research. Having FLRWSolver included in the ET would be useful for me and others doing research in numerical relativistic cosmology.
-
reporter A thornlist stanza to check out FLRWSolver can look like this:
# FLRWSolver # strange path is due to repo name !TARGET = $ARR !TYPE = git !URL = https://github.com/hayleyjm/FLRWSolver_public.git !NAME = FLRWSolver !REPO_PATH= ../$2 !CHECKOUT = EinsteinInitialData/FLRWSolver
where I mimic the naming convention (up to capitialization) suggested in README.Compilation to name the checkout FLRWSolver rather than FLRWSolver_public (this also makes the stanza using a bit less strange REPO_PATH).
Currently (without running bulder.py) fails to compile so is not “harmless” to include in master (since it prevents the thornlist from compiling).
-
reporter Trying to run `builder.py` I find that it requires the cffi package which should be mentioned (as well as any other dependencies not in the base Python set of packages).
-
reporter - changed milestone to ET_2021_11
-
reporter Has there been any progress on this?
-
reporter Has there been any progress on this?
-
reporter With the release out in November it is now time again to start looking into this seriously. Hayley do you need any help bringing the code up to readiness? Helvi would you like an additional reviewer to help out with reviewing the code?
-
reporter - changed milestone to ET_2022_05
-
reporter -
assigned issue to
@Zach Etienne assigning to you as the new reviewer.
-
assigned issue to
-
reporter Draft review guidelines are at https://docs.einsteintoolkit.org/et-docs/How_to_Review_a_new_component
-
reporter Pierre Mourier may be able to help with review.
-
reporter Pierre is not available due to time constraints, same for Michele. Hayley and Roland looking for more reviewer candidates urgently.
-
reporter Review has started, Zach is in contact with Hayley, who did some code cleanup.
-
reporter Chi Tian agreed to provide science level input if needed.
-
I’ll add comments as I have time.
Issue 1: Initial data thorns should depend on *Base thorns, and not any particular evolution thorn. I’m referring to
shares:GRHydro USES real rho_abs_min USES real rho_rel_min USES REAL initial_rho_abs_min USES REAL initial_rho_rel_min USES REAL initial_atmosphere_factor USES real GRHydro_rho_central
within
param.ccl
. It should be the initial data thorn’s responsibility to set up the atmosphere and set its own atmosphere parameters accordingly. Then it’s up to the user to set evolution thorn parameters such that this atmosphere is either maintained or replaced with a different one. For me, the solver wouldn’t even get to the compile stage because I generally comment outGRHydro
from myThornList
(naturally I useIllinoisGRMHD
).Oh, I also checked and confirmed that the only other thorn with
shares: GRHydro
isGRHydro_InitData
, which seems appropriate. -
Issue 2: Compiler warning needs to be resolved (looks scary):
src/powerspec_ics.F90:16: Warning: While tracing include dependencies: Include file "fftw3.f03" not found
-
Issue 3: The
test/
directory contains a parameter file and sample output data, which has no problem running and seems to use little resources. Great work! What’s missing is the requiredtest.ccl
file that sets acceptable tolerances when running this thorn through our unit testing infrastructure. In fact I’m pretty sure thattest.ccl
must exist here for the directory to even be seen by our unit testing infrastructure. -
Issue 4:
doc/cactus.sty
should be removed. I understand that this is a convenient location for the file when running e.g.,pdflatex documentation.tex
from within thedoc
directory, but this file already exists in the Toolkit in a standard place that is looked at when e.g.,make AllDoc
is run. It's best that this remains the situation in case we'd like to update the style file. -
Issue 5: I’m a bit confused as to why
check_metric()
issomething that we do at the end of each initial data routine
. All it does is:if (CCTK_EQUALS (metric_type, "physical")) then ! do nothing else call CCTK_WARN (0, "Unknown value of ADMBase::metric_type -- FLRW only set-up for metric_type = physical") endif
This parameter cannot change while
FLRWSolver
is being run, or at all after the parfile is read in, so it only needs to be checked once. In fact I think there’s a scheduling bin for it:ParamCheck
. -
Comment 1: I really like
README.Compilation
as it contains some useful “gotchas” that folks often run into when trying to compile the Toolkit more generally. For example:--> Generally, if you have a nontrivial error with a particular thorn, first try removing that thorn from the thornlist and re-compile. The code will complain if other thorns on your thornlist depend on the thorn you have just removed. You can also remove some of these if they are not important. If it turns out a thorn you really need has a dependency on that thorn, ONLY THEN spend the time debugging.
I feel like the contents of this file would make an excellent contribution to a wiki page.
-
Issue 6: I couldn’t find reference to the example power spectrum (see
powerspectrum/
) in the documentation. This would be a useful addition. Also I liked that when I went intopowerspectrum/
I was greeted with aREADME
file explaining the basics of what this was. -
Comment 2:
documentation.tex
only refers to one Python script intools
, despite there being 5 scripts in that directory and a Jupyter notebook. It would be so nice to reference these in the documentation, as they are likely to be very useful to new users. Also I really like that there’s a tutorial! -
Issue 7: There are two, slightly different, copies of the
Plot_ET_data.ipynb
tutorial in the repo: one indoc/tutorial
and one intools/
. This could make the thorn more difficult to maintain moving forward. -
Comment 3: Instead of storing simfactory configs for machines within a thorn’s repo, consider contributing them to
SimFactory
. -
Issue 8: There are two
FLRW_singlemode.par
files: one inpar/
and one intest/
. These seem to be two very different files, so adjusting the filenames to be more descriptive would help new users browsing these example parameter files. -
Issue 9: Running
par/FLRW_powerspectrum.par
results in a FORTRAN runtime error, as/path/to/flrwsolver/powerspectra/FLRW_matterpower_synchronous_z1000.dat
does not exist. It would be better to check for the file’s existence in the Einstein Toolkit CCTK_*ERROR infrastructure, and would be a great opportunity to tell the user how to generate this file. -
Issue 10: A more detailed description of what
par/FLRW_powerspectrum_restart.par
does in that file would be appreciated. It’s rather unusual and probably violates ET standards to have a separate parameter file just for restarting from checkpoints. -
Issue 11:
random.F90
contains a code snippet “found on stackexchange”, but does not contain a link to the SE article. Complete attribution needed. -
Comment 4: I’m rather confused as to why
random.F90
exists. It seems to be a custom RNG. Why not use something off the shelf? -
Issue 12:
init_tools.F90
contains routines from another library “MESCALINE” but does not contain a link to the library or author attribution. Possible software license violation. -
Hi all, I have reviewed the documents and majority of the the codes. At least at physics level, everything looks fine to me. Only some tiny issues regarding the way FLRWSolver dealing with the extrinsic curvature:
- It seems there is a typo in Eq. (10) in the document, where minus sign is missing (at least for the case where the author mentions “Setting f =1/3 and n=2 in the above yields ∂tα=∂ta for EdS“).
- The name of the variable
kvalue
in severalflrw_*.f90
files seems a little confusing. From its name, it seems this variable represents the extrinsic curvature K. However, its true value seems to be3 * kvalue
if my understanding was correct. - By following the gauge condition
\dot{\alpha} = f * K
, effectively it is setting the coordinate time to something closed to the conformal time\eta
, and the value of\alpha
will grow roughly following the growth of the scale factora
. Could this possibly introduce numerical instabilities toGRhydro
when starting from a very high redshift, and universe keeps expanding for quit long? Since I would guess most of astrophysical GR simulations may choose gauge conditions that make\alpha
close to 1.
-
reporter @Zach Etienne confirms “does no harm” in http://lists.einsteintoolkit.org/pipermail/users/2022-August/008670.html
-
reporter I have added FLRWSolver to the main ET thornlist as:
# FLRW cosmological initial data !TARGET = $ARR !TYPE = git !URL = https://github.com/hayleyjm/FLRWSolver_public !REPO_PATH = ../$2_public !CHECKOUT = FLRWSolver/FLRWSolver
which checks out
FLRWSolver_public
and creates a thornFLRWSolver/FLRWSolver
for it.If you would rather use a different arrangement or thorn name, please let me know.
-
Thanks Roland! I usually put FLRWSolver in EinsteinInitialData/FLRWSolver, but whatever you think works best is fine by me.
-
reporter Sure. I can move it. Some groups like the “branding” of the arrangement. Any location is fine with me though (well except the Cactus* locations which come with special license requirements). I will update the thornlist.
@Zach Etienne the warning you saw was that:
/data/rhaas/postdoc/gr/cactus/ET_trunk/arrangements/FLRWSolver/FLRWSolver/src/powerspec_ics.F90:16: Warning: While tracing include dependencies: Include file "fftw3.f03" not found Searched in thorn directory and in [/data/rhaas/postdoc/gr/cactus/ET_trunk/configs/sim/build/FLRWSolver/../FFTW3, /data/rhaas/postdoc/gr/cactus/ET_trunk/configs/sim/build/FLRWSolver/../SpaceMask] Checking status of thorn IDBrillData
? If so the most likely it can be ignored. It is due to Cactus trying to set up makefile dependencies for include files in Fortran code. Those are maybe non-trivial (and may not correctly take ExternalLibraries into account, being rarely used). If that is the one then it is no showstopper though I will try and take a look (reluctant as I am to change anything in that part of CST, fearing ripple effects if I touch anything…).
-
@Roland Haas : Yes that was the warning I saw. Probably innocuous but better to be safe than sorry, by looking into it.
-
@Zach Etienne thanks for your review! I have addressed all of the issues and comments you have raised, and pushed the changes to the repo.
In addition to the changes to address your issues/comments, I have also re-named the source files and routines themselves to be more consistent with ET standards.
Here I’ll explain what I’ve done:- Issue 1: removed these statements from param.ccl as they were unused anyway
- Issue 2: Roland said he would have a look into this, so I’ll leave this be for now
- Issue 3: added a test.ccl file
- Issue 4: removed doc/cactus.sty
- Issue 5: thanks for this! I’ve made a new routine FLRW_ParamCheck and scheduled it at ParamCheck as you suggested, and removed this code from the ID routines
- Issue 6/Comment 2: I’ve added a comment on the example powerspectrum file in the documentation, and added a new section explaining each of the files in the tools/ directory.
- Issue 7: I removed doc/tutorial/ and moved the README.tutorial file to tools/
- Comment 3: I removed configs/ from the public repo and will consider contributing these to simfactory at some point
- Issue 8: re-named the test par to FLRW_singlemode_test.par since the main differences are the resolution and simplified initial parameters
- Issue 9: thanks for pointing this out. I’ve added a call to CCTK_WARN after checking if the supplied file exists, with a useful error explaining next steps for the user
- Issue 10: removed the restart par file from par/
- Issue 11/Comment 4: thanks for this - I realised random.F90 was a lot more complicated than it needed to be. It already uses the intrinsic fortran PRNG, but had some added complexity in generating a seed if it wasn’t given. In FLRWSolver, there will always be a random seed. I removed random.F90 and replaced it with a new, much simpler routine in FLRW_InitTools which directly calls the Fortran intrinsic PRNG and casts this into random numbers following a normal distribution
- Issue 12: Mescaline is my private post-processing code, so I’ve removed this comment
Thanks again for your help and effort on this! Let me know if there’s anything else or if you have questions about my changes.
-
Chi - thanks for your review! I’ve addressed your comments, see below:
1. Thanks for this, you’re right there was a minus sign missing which I’ve now fixed
2. I’ve changed the name of this variable from kvalue → kdiag_bg, since it represents the diagonal value of the extrinsic curvature K_ij for the background (not the trace)
3. Yes, you are correct that this choice of gauge means that for the background we have alpha → a and therefore coordinate time can be more or less interpreted as conformal time. I don’t think this choice of evolving lapse will give numerical errors (which I don’t see, as far as I can tell), because the actual time evolution is dependent on the time step in terms of coordinate time, dt, which is set via the grid spacing and the chosen Courant factor. This time step does not change as the lapse evolves, and I don’t think the lapse will have an impact here. I could be wrong, so @Roland Haas please correct me if so!Thanks again for your time and for helping with this on such short notice!
-
reporter Just catching up with things (for half a day and then I will falling behind again), I cannot comment on the actual physics. Certainly the description of the Courant factor is correct. There are certainly bits and pieces in the ET that will make assumptions like “lapse is about 1 at large radii” but those are mostly say gravitational wave extraction which is less useful in this situation (since it also implicitly assumes a Schwarzschild like background for the waves).
Having a large lapse should be fine, if I am not mistaken then eg Zach has a trick to smooth out initial gauge dynamics by temporarily setting a very large lapse even for black hole simulations (though likely only near the black holes). Anyway, the existing codes in the ET should (fingers crossed) all handle 0 < lapse < infty at least in cases where this makes sense.
-
reporter FLRWSolver is included in the ET as of the 2022_11 Kowalevski release: https://www.einsteintoolkit.org/about/releases/ET_2022_11_announcement.html
-
reporter - changed status to resolved
- Log in to comment