Skip to content

Feedback overhaul#386

Open
mabruzzo wants to merge 165 commits intocholla-hydro:devfrom
mabruzzo:s99_f-feedback
Open

Feedback overhaul#386
mabruzzo wants to merge 165 commits intocholla-hydro:devfrom
mabruzzo:s99_f-feedback

Conversation

@mabruzzo
Copy link
Collaborator

@mabruzzo mabruzzo commented Mar 26, 2024

This should be reviewed after #418 and #420 are merged


This overhauls stellar feedback and makes a few tweaks to the gravity solver to make it possible to run with particles so that the particles feel self-gravity and the fluid only feels a static potential.

mabruzzo added 30 commits March 25, 2024 15:09
…the treatment Temperature floor in CIE case.
Specifically, this commit converted the stellar-wind initializer so that
it now uses the new parsing functions. Note, that I wasn't anywhere near
as careful as I was while converting the supernova initializer. I
literally just made sure the converted code compiled.

For context, when I converted the supernova initializer, I took care to
ensure that the results were bitwise identical.
While everything compiles just fine, I wasn't as careful as I was when
factoring out the SNRateCalc class. For context, when I factored out
the SNRateCalc class, I took care to ensure that there were no
alterations to any of the outputs. In contrast, for the SWRateCalc class
I didn't actually check any runs that employ stellar winds (although
they definitely do compile correctly).
constructors for SNRateCalc and SWRateCalc.
container:
#- {name: "CUDA", link: "docker://chollahydro/cholla:cuda_github"}
- {name: "HIP", link: "docker://chollahydro/cholla:rocm_github"}
- {name: "HIP", link: "ghcr.io/cholla-hydro/cholla-rocm:sha-a08bd2c"}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this intentional? For some reason I thought this had been updated elsewhere.

@@ -1 +1 @@
MPI_GPU = -DMPI_GPU
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a brief comment to the top line of this makefile (and the starblast) just stating the basic purpose (like in the hydro and mhd files)?


### Primer on Radiative Cooling

At the time of writing, simulations must always be initialized with CIE cooling (cooling shuts off below 1e4 K) in order to prevent immediate of the gas disk. The current strategy for using other cooling mechanisms. After running the simulation long enough to start driving turbulence in the gas disk (commonly a few 10s of Myr), you can then restart the simulation with a different cooling recipe.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: "immediate collapse of the..."

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also the following sentence is not complete.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed


### Primer on Feedback and Star Formation:

In a simulation without particles, we feedback is implemented with a CGOLs like mechanism.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This option is not actually included in this version of cholla, right? My recollection is that the CGOLS feedback code has only ever existed in branches that have not been merged into this repo, in which case, maybe we should not mention it here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I think so. (It wasn't clear to me back when I first wrote these docs)

:::

At the time of writing, we always define {math}`\Phi_{\rm stars,old}(R,z)=\Phi_{\rm MN}(R,z; M_{\rm stars}, R_{\rm stars}, z_{\rm stars})`, where {math}`R` & {math}`z` are cylindrical coordinates, {math}`R_{\rm stars}` & {math}`z_{\rm stars}` are the scale radius/height, and {math}`M_{\rm stars}` is the mass of the disk.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible in this section to give a brief description of how these parameters are set? I.e. are they hardcoded in the disk model, are they runtime parameters...?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its hardcoded (I added a description)

:::

In the above equation
- Consequently, we define . At the time of writing, this **never** includes contributions from self-gravity. (this definition of self gravity means that we can **NEVER** achieve perfect )
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple missing phrases.

:::

At startup we sample this PDF to determine the {math}`r_{\rm cyl}` at which all particles are expected to form. We create star-particles at these radii and distribute the "turn-on times" from a time a little before we start the simulation until the time specified by the `tout` parameter. We use Poisson sampling to distribute these "turn-on times," to target a particular SFR.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar comment here about how the SFR is set. Is this a runtime parameter, or hardcoded somewhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its hardcoded. I added a brief description

The main alternative that we could revisit is implementing the method prescriptions as atomic operations. If we want to adopt this solution in the future, there are a number of important considerations that need to be addressed in the future. These details are highlighted under the collapsible tag


#### ASIDE: Important considerations for atomic "conflict-resolution"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this supposed to be collapsible?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. The section was originally collapsible on the GitHub wiki.

With the switch to sphinx, we lost the ability to do that (so I just discuss things in the next subsection). We could probably find a sphinx extension to do that (but it may not be worth doing).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants