search

grackle-project / grackle

The Grackle chemistry and cooling library for astrophysical simulations and models.
Other
26 stars 50 forks source link

Grackle delivers inconsistent results when run with and without comoving coordinates #192

Closed mladenivkovic closed 2 months ago

mladenivkovic commented 6 months ago

I have stumbled upon this issue trying to add cosmological integration to my own code. To debug, I have extracted the grackle part into standalone grackle mini-programs, that only call the grackle solver.

The setup is as follows: I take a single cell of hot, ionized gas and let it cool down from redshift 20 to 4. I use primordial chemistry 1, no UV background, no specific/volumetric heating rates, no metal cooling tables:

  grackle_chemistry_data->use_grackle = 1; /* chemistry on */
  grackle_chemistry_data->with_radiative_cooling = 1;
  grackle_chemistry_data->primordial_chemistry = 1;
  grackle_chemistry_data->dust_chemistry = 0;
  grackle_chemistry_data->metal_cooling = 0;
  grackle_chemistry_data->UVbackground = 0;
  grackle_chemistry_data->grackle_data_file = "";
  grackle_chemistry_data->use_radiative_transfer = 0;
  grackle_chemistry_data->HydrogenFractionByMass = 0.76;
  grackle_chemistry_data->Gamma =   (5. / 3.);

I solve the problem in 2 ways (each as a separate program/mini-app). Once, I provide grackle with quantities in co-moving coordinates and set the comoving_coordinates flag. I also adapt the grackle units according to the documentation.

In the second version, I provide grackle with proper coordinates, and turn comoving_coordinates off. Additionally, I manually evolve the gas quantities with the expansion by applying the appropriate multiplications with the scale factors.

To make sure that is all correct, I ran the experiment without calling the grackle cooling solver. The outputs are identical:

cosmo_cooling_comparison-NO_SOLVER

Problems start when I actually call the grackle cooling solver:

1e11

Things I have tried:

At this point, I'm quite convinced this is an issue on grackle's side. The mini-apps I'm running are available here: https://github.com/SWIFTSIM/swiftsim-rt-tools/tree/add_cosmo_cooling_test/grackle/CosmoCoolingTest

(Please note that the setup is not in the main branch of that repository, but in the add_cosmo_cooling_test branch. The link I posted above points to that branch already.)

mabruzzo commented 6 months ago

Thanks for reporting this. I'll try to dig into this a little later this week.

mabruzzo commented 3 months ago

@mladenivkovic Sorry for taking so long to get back to you.

We've recently been discussing Grackle's unit invariants (see #216) and I suspect that is related to the issue you are seeing. Specifically, Grackle assumes a comoving code unit-system where the units of velocity fields and specific energy are constant with respect to scale factor. In contrast, I believe the system you are using has internal energy vary with respect to scale factor.

mladenivkovic commented 3 months ago

Hi Matthew

No, I don't think that's the case. In the mini-app where I use cosmological units (https://github.com/SWIFTSIM/swiftsim-rt-tools/blob/add_cosmo_cooling_test/grackle/CosmoCoolingTest/main.c), I set the internal energy once on line 249, and then don't touch it ever again. Any changes to it is done by grackle.

Conversely, in the case where I use proper units (https://github.com/SWIFTSIM/swiftsim-rt-tools/blob/add_cosmo_cooling_test/grackle/CosmoCoolingTest/main_physical.c), I do need to modify the internal energy manually to account for the expansion. However, in that case I'm not running grackle with cosmology enabled, and grackle is provided with the proper values of the gas quantities.

mabruzzo commented 3 months ago

Right, I don't think I'm being very clear.

Enzo's comoving unit-system (this is basically the only comoving system that Grackle's comoving-support is directly compatible with[^1]) makes a somewhat unintuitive choice:

Basically to get your cosmology example to work, I think you would need to convert the internal energy field from comoving to proper before calling grackle and then back again afterwards to do hydro/gravity/other stuff.

Does this make more sense? (It's also possible I'm misunderstanding what you're doing)

I do have some ideas for making grackle's comoving support easier to use. It might also be possible to add support down the line for other comoving unit systems (down the road) if you think that would be useful.

[^1]: This is a new development from Issue #216 -- we only recently figured this out. I plan to update the docs accordingly. [^2]: It would be a little more intuitive if velocity units, length units, and time units were related by $VU = LU / TU$, which I think is what swift assumes, based on the function call on Line 245. But the actual relationship is $VU = LU / (TU \times x)$ -- this affords some convenient advantages for heating/cooling.

mabruzzo commented 3 months ago

@mladenivkovic as a brief aside, I noticed a little while back that you were maintaining a spack package for Grackle (here, I think).

I don't have much experience with spack (or 3rd part package/dependency managers in general), but I care a lot about making Grackle easier to to consume. If you and other people would find spack support to be useful, would it be helpful if we took steps to add support to the main Grackle repository?

mladenivkovic commented 3 months ago

Hi Matthew

I tried storing the internal energy permanently in proper units. In my mini-examples, it doesn't get touched anywhere else aside from applying the correct expansion factor to dilute it as time progresses.

Unfortunately, still no dice: cosmo_cooling_comparison

Regarding spack: The package you found is one I added out of convenience for me. It installs the specific frozen grackle fork that I use with SWIFT. Basically, I'm pretending it's a new package "GrackleSwift" that installs my fork.

Grackle is already provided as a spack package. It usually installs without any issues. I copied that package data and slightly modified it to get the one for my use. If you would like to add newer versions of grackle to spack, I'm happy to point you to what needs to be added (provided they haven't changed the syntax in the meantime, spack is still under development and does that sometimes....) The way I was doing it, it required to modify 2-3 lines in the package.py file.

gregbryan commented 3 months ago

Hi Mladen — I think Matthew is correct about the energy code units (which are comoving in the sense that they don’t need to be diluted with expansion).

Your revised version looks much better but the cosmology one cools a bit faster. I suspect this is due to the Compton cooling off the CMB, which is always turned on when cosmology is activated. The Compton cooling time above z>6 is shorter than the age of the Universe, so I think the faster cooling is expected given that assumption. Unfortunately, we don’t have a flag to turn that off (the thinking was that the CMB is a non-optional part of cosmology but we could add a flag). As a quick test, you could try the same integration from a lower redshift (I think z=3.2 to z=0 would be the same expansion period as z=20 to z=4) and you should see much more similar evolution. To fully test it properly, you could hack comp_rate in src/clib/rate_functions.c to return 0.

Hope that helps, Greg

On Jul 9, 2024, at 4:38 PM, Mladen Ivkovic @.***> wrote:

Hi Matthew

I tried storing the internal energy permanently in proper units. In my mini-examples, it doesn't get touched anywhere else aside from applying the correct expansion factor to dilute it as time progresses.

Unfortunately, still no dice: cosmo_cooling_comparison.png (view on web) https://github.com/grackle-project/grackle/assets/15235823/de9c41e6-c9d0-4d57-93d8-11e7a7fcd238 Regarding spack: The package you found is one I added out of convenience for me. It installs the specific frozen grackle fork that I use with SWIFT. Basically, I'm pretending it's a new package "GrackleSwift" that installs my fork.

Grackle is already provided as a spack package. It usually installs without any issues. I copied that package data and slightly modified it to get the one for my use. If you would like to add newer versions of grackle to spack, I'm happy to point you to what needs to be added (provided they haven't changed the syntax in the meantime, spack is still under development and does that sometimes....) The way I was doing it, it required to modify 2-3 lines in the package.py file.

— Reply to this email directly, view it on GitHub https://github.com/grackle-project/grackle/issues/192#issuecomment-2218689734, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABQBD6FS6F7WTO6S3KH6GALZLRC4BAVCNFSM6AAAAABGD4CPHWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMJYGY4DSNZTGQ. You are receiving this because you are subscribed to this thread.

Greg Bryan Professor of Astronomy, Columbia University

mladenivkovic commented 3 months ago

Hi Greg

Thanks for the reply! Running from z~3.2 to 0. indeed gives the same results.

cosmo_cooling_comparison

gregbryan commented 3 months ago

Great!

On Jul 11, 2024, at 11:58 AM, Mladen Ivkovic @.***> wrote:

Hi Greg

Thanks for the reply! That indeed fixes it.

cosmo_cooling_comparison.png (view on web) https://github.com/grackle-project/grackle/assets/15235823/00ed5164-e641-49dc-8709-6a2853e6cf93 — Reply to this email directly, view it on GitHub https://github.com/grackle-project/grackle/issues/192#issuecomment-2223312932, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABQBD6AWGCIFKYA2SNDWLATZL2TRDAVCNFSM6AAAAABGD4CPHWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMRTGMYTEOJTGI. You are receiving this because you commented.

brittonsmith commented 2 months ago

Great to see this has been sorted! Shall we close this issue?