Cosmo ics

[brantr]

This update provides several additions to the cholla code base:

• Cosmological initial conditions generation. Given an input power spectrum, the initial dark matter particle positions and velocities and gas cell densities and velocities are generated. Currently uses the Zel'dovich approximation but can be extended to higher order perturbation theory.
• Adds infrastructure for creating parallel random numbers (used in the cosmo ICs)
• Adds infrastructure for using Henry as an FFT platform separate from Paris
• Adds infrastructure for ghost cell communications for individual fields separate from the bulk Cholla communication routines.
• Adds differential equation solver for computing the cosmological growth function (including for w0-wa cosmologies)
• Ports in the OTVET radiative transfer scheme from the RT branch and has a developmental version of M1 RT.
• Addresses bugs in the MPI_GPU communication for particles when communication buffers need to be resized.

[brantr]

pre-commit.ci autofix

[brantr]

So, I think I finished skimming through most everything (I haven't carefully traced out all of the detailed logic -- but nothing egregious jumps out to me). In all of my new comments/suggested changes, I tried to emphasize what I thought was particularly important. As per usual, feel free to push back on comments you disagree with.

I haven't looked at the Altair stuff or the RT example plotting scripts, but I'm not really planning to look at that.

Thank you for your efforts.

I'm not thrilled about all of the stuff in the examples/3D/illev directory. I am most irked by the images that are saved there, and we should probably consider a better place for storing the plotting scripts. But, given the experimental nature of RT (and the fact that quite a bit of work is probably required to rearrange the files while ensuring that they continue to work), I'm will to let this go.

We can move these items, but they are extremely helpful for diagnosing performance. I would recommend pushing this issue to the M1 merge.

I would appreciate it if you would at least consider a suggestion for changing the architecture of the code that sets the Cosmo ICs.

I think this is a design consideration that would require some effort. I will not have time for this until at least September.

To be clear, I think I'll be ready to approve the PR once you address the new comments (regardless of whether or not you actually adopt my suggested alternative Cosmo IC Architecture).

Thank you for your efforts during review and your support, it's been extremely helpful.

Alternative Cosmo IC Architecture

At a high-level, the suggestion is quite simple: what if we created a new class that was responsible for all of the cosmology initialization logic?

For the sake of this discussion, let's call this class CosmoICMaker. In more detail, we would move all of the Cosmo IC methods (other than Grid3D::Cosmological_ICs) and all of the associated data members (CP, rng_states, fft) into the new CosmoICMaker class.

I would disagree that rng_states and fft should be part of this isolated structure. These could be generally useful.

There's obviously already a lot of precedence for doing something like this in the codebase. For example, Particles3D is responsible for managing the deposited density field, Grav3D is responsible for managing the potential field, and Rad3D is responsible for managing all the RT fields. I'm frankly not a fan of this kind of distributed ownership of fields.1 However, I think the ephemeral nature of the Cosmo IC fields (i.e. we explicitly allocate and deallocate them before running the entire simulation) makes a very compelling case for treating them distinctly from all other fields in the codebase.

I agree that the ephemeral nature of the fields make them amenable to special treatment.

I think this change is a good idea for 2 related (and potentially obvious) reasons:

1. The Grid3D object is unfortunately a "god object," which is universally recognized as an anti-pattern.2 As we add more and more functions and data members, we start to introduce lots of the issues associated with using global variables. In particular, it becomes harder and harder to reason about any one small section of the codebase in isolation. Just the sheer number of methods is problematic.3 Thus, we should be trying to delegate as much logic as possible to helper classes to both (i) make it easier to reason about that new logic and (ii) avoid making the problem even worse. If we don't factor out this logic, we would be adding ~20 new methods.

I agree that we could work on this in the future.

2. Less importantly, I'm not a fan of a class having lots of initialization methods. It essentially means that class has more and more valid partially initialized states that we need to reason about. If we create a CosmoICMaker class, then from the perspective of the Grid3D class, I think we only need to worry about "is the CosmoICMaker instance valid or not?" The precise details of what it means to be in a valid state would then be handled by the CosmoICMaker class (largely in isolation of the rest of Grid3D's state).

I agree.

Beyond the up front inconvenience, I don't actually see a compelling reason not move this logic into a new CosmoICMaker class. Since you have already written a bunch of new boundary update logic, I don't think this would even be that much of the inconvenience.4 While I can think of some potential performance benefits to the current design, they all seem pretty negligible relative to the cost of actually driving the Cosmic IC logic. Am I missing something?

No, I don't disagree.

I actually feel VERY strongly about this topic. In fact, if this were a normal PR, I would probably insist that this change (or something like it) be made before the PR could be accepted5 or to be convinced that the current design makes more sense. I honestly wouldn't care that much if this logic weren't in such a central data structure of the codebase.

I think we are just person-hour limited at this time. I'm open to revisiting this in September.

However, the reality of the situation is that we really want to merge in all the other changes in the PR, so I'll let this go if you don't want to do it.

I do want to do it, we just have time constraints over the next four weeks on the cosmo ICs + RT issues that will require concerned development effort on the physics.

I am happy to meet next week to discuss.

[evaneschneider]

I'm happy to merge this in once the new Makefile parameters (the ones added in src/cholla_config.h.in) and input file parameters have been added and defined on the relevant docs pages https://cholla.readthedocs.io/en/latest/MakefileParameters.html and https://cholla.readthedocs.io/en/latest/Reference/Parameters.html. Thank you for the documentation on the ICs generation.

[evaneschneider]

This is now triggering the clang-tidy error "statement should be inside braces"

[brantr]

Fixed, thank you.

[evaneschneider]

This line (22) is still triggering the clang tidy error. I think you need braces on line 22 (and probably 23 as well).

[evaneschneider]

It looks like maybe there's an accidental duplication here as well?

[brantr]

@evaneschneider I have added to the MakefileParameters.md and Parameters.md files. I have tried to briefly document all the existing un-documented makefile parameters (including old ones) and added descriptions of all the new parameters for the cosmological simulations.

[evaneschneider]

Looks good, thank you. I'll merge this in once the CI tests finish.