Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Light refactor of EnzoMethodPmDeposit::compute and gas-deposition drift-time bugfix #239

Merged
merged 4 commits into from
Jul 1, 2022

Conversation

mabruzzo
Copy link
Contributor

This PR was primarily written to implement a bugfix (relevant to PR #187), but I got a little carried away with some refactoring.

The first 2 commit lightly refactors EnzoMethodPmDeposit::compute. I have summarized these changes in the following spoiler tag.

Refactoring changes
  • the method is now broken up into some local helper functions, which should make the logic easier to understand. Previously the method was 430 lines long and it was a little difficult to determine which variables used for Particle-mass deposition affected the gas-mass deposition.
  • removed some unnecessary heap-allocations from the gas deposition
  • transitioned the heap allocations to make use of std::vector and CelloArray (so that we don't need to explicitly deallocate the pointers) and could drop some calls to std::fill_n
  • dropped an unnecessary call to std::fill_n
  • started to use CelloArray in some places (calls to field::view does more and louder error-checking than field::values)
  • made it more explicit that the array filled by dep_grid_cic only includes cells for the active zone.

The primary motivation for this PR was to address a bug related to the gas-deposition drift-time. Previously this drift time was set to the value of alpha (taken from Method:pm_deposit:alpha). As @stefanarridge pointed out in PR #89 this didn't make a lot of sense. I also ran into issues with introducing a Stable Jeans Wave regression test in PR #186 that required me to set Method:pm_deposit:alpha to 0.

For comparison, the drift time for depositing gravitating mass particles is set to alpha * dt / cosmo_a, in which:

  • alpha again comes from Method:pm_deposit:alpha
  • dt is the time-step during the current cycle
  • cosmo_a is the scale factor computed at time + alpha * dt (where time is the time at the start of the current cycle)

To investigate this issue I checked the original code from enzo-dev in Grid_DepositBaryons.C. I concluded that enzo-dev sets the gas density's drift-timestep should get set in exactly the same way as the Particle's drift timestep. However, when using the PPM and Zeus Hydro-solvers, the drift timestep is set to 0 (this is consistent with a comment @johnwise made during his review of PR #89). Since our 2 primary hydro-solvers in Enzo-E (Ppm and VL+CT) currently handle these acceleration source terms with the same temporal order as the solvers, we now force the gas-deposition drift timestep to be 0 in EnzoMethodPmDeposit.

@mabruzzo mabruzzo added the bug Something isn't working label Jun 27, 2022
@mabruzzo mabruzzo added this to the v1.0 milestone Jun 27, 2022
@mabruzzo
Copy link
Contributor Author

For the Enzo experts out there, I did have a minor clarifying question about lines 204-206 of Grid_DepositBaryons.C.

Does dxfloat always give the cell-widths in comoving-distances or proper distances? Currently, the equivalent code in EnzoMethodPmDeposit explicitly converts the cell-widths to always be proper distances (by dividing out the scale factor).

If you don't have time to look this up, we can punt this question to PR #187

mabruzzo added 2 commits June 27, 2022 09:41
…posit

Previously the drift time for the gas density was set to the value of `alpha` (taken from `Method:pm_deposit:alpha`). As @stefanarridge pointed out in PR enzo-project#89 this didn't make a lot of sense. I also ran into issues with introducing a Stable Jeans Wave regression test in PR enzo-project#186 that required me to set `Method:pm_deposit:alpha` to 0.

For comparison, the drift time for the gravitating mass particles is set to `alpha * dt / cosmo_a`, in which:
- `alpha` again comes from `Method:pm_deposit:alpha`
- `dt` is the time-step during the current cycle
- `cosmo_a` is the scale factor computed at `time + alpha * dt` (where `time` is the time at the start of the current cycle)

To investigate this issue I checked the original code from enzo-dev in `Grid_DepositBaryons.C`. I concluded that enzo-dev sets the gas density's drift-timestep should get set in exactly the same way as the Particle's drift timestep. However, when using the PPM and Zeus Hydro-solvers, the drift timestep is set to 0 (this is consistent with a comment @johnwise made during his review of PR enzo-project#189). Since our 2 primary hydro-solvers in Enzo-E (Ppm and VL+CT) currently handle these source terms with the same temporal order as these solvers, we now force the gas-deposition drift timestep to be 0 in `EnzoMethodPmDeposit`.

We will need to revisit this exact behavior when we eventually modify the VL+CT solver to handle self-gravity in a higher temporal-order manner.
@fearmayo fearmayo self-requested a review June 29, 2022 10:23
@peeples peeples requested a review from gregbryan June 30, 2022 12:20
@gregbryan
Copy link
Contributor

For the Enzo experts out there, I did have a minor clarifying question about lines 204-206 of Grid_DepositBaryons.C.

Does dxfloat always give the cell-widths in comoving-distances or proper distances? Currently, the equivalent code in EnzoMethodPmDeposit explicitly converts the cell-widths to always be proper distances (by dividing out the scale factor).

If you don't have time to look this up, we can punt this question to PR #187

I'm almost certain those are comoving cell widths, not proper, so the multiplication by a does not match Enzo. However, note that this doesn't impact anything if dt is zero for the baryon deposit because those cell widths are only used in grid_cic.F by multiplying by dt (so since we have set dt=0 here, it doesn't matter what we pass in for the cell widths). This should probably be cleaned up at some point, but I'm happy punting this to a future PR.

@fearmayo fearmayo merged commit 7b6acd6 into enzo-project:main Jul 1, 2022
@mabruzzo mabruzzo deleted the pm-deposit-fix branch July 1, 2022 13:13
mabruzzo added a commit to mabruzzo/enzo-e that referenced this pull request Jul 7, 2022
PR enzo-project#239 introduced a minor bug in which array used for storing the output of dep_grid_cic was not large enough. This caused dep_grid_cic to write values to invalid locations in memory. This bug manifested in weird ways when using compiling with intel compilers (under special conditions)

In addition to fixing that bug, some variable names were updated so that their significance became more apparent
WillHicks96 pushed a commit to WillHicks96/enzo-e that referenced this pull request Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants