Skip to content

Full implementation of AGN feedback for the cool-core destruction project #339

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.

Sign up for GitHub

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

Jump to bottom
Open
ShuangShuang0411 wants to merge 193 commits into
base: main
Choose a base branch
from
Open

Full implementation of AGN feedback for the cool-core destruction project #339

ShuangShuang0411 wants to merge 193 commits into from

Conversation

@ShuangShuang0411
Copy link

@ShuangShuang0411 ShuangShuang0411 commented Jul 18, 2024

This pull request includes the complete implementation of AGN feedback for the cool-core destruction project. The key components are:

  1. The AGN feedback module in the ClusterMerger test problem.
  2. The ExactCooling test problem for the currently private ExactCooling source term module.

@hyschive
Copy link
Contributor

hyschive commented Jul 20, 2024 •
edited by ChunYen-Chen
Loading

@ShuangShuang0411 Some quick and general comments. I'll check the entire PR thoroughly after it is approved by other reviewers.

  • Make sure that this PR can reproduce the original results of the main branch, including but not limited to the ClusterMerger test. For example,
    • The line # define NCOMP_PASSIVE_BUILTIN2 1 in Macro.h (and other similar lines/files related to cooling). should be activated only when enabling cooling. You may need to add a compilation option in configure.py to control the cooling module. Ask @ChunYen-Chen for details.
    • You have slightly modified some parameters in Input__Parameter (e.g., MU_NORM, MIN_DENS, ...). @jzuhone What do you think about this? If you prefer, we can have different Input__Parameter files (in the same ClusterMerger folder) tailored for different applications.
  • Remove all trailing spaces
  • Replace all tabs with spaces
  • Polish the code format further to conform better with the GAMER style (e.g., indent, code alignment, add empty lines for separating different code segments, add more comments)
  • Fix all the CPU and GPU warning messages during compilation
  • IIRC, dual-energy is important when enabling cooling and/or feedback. Clarify that in README (along with other things that can be important; for example, how to conduct simulations with cooling and feedback in general).

@hyschive
Copy link
Contributor

hyschive commented Jul 20, 2024 •
edited
Loading

@jzuhone Since this PR is most related to your work, could you help review it? I'll also take a close look after you approve it.

First, do you prefer to modify the existing ClusterMerger test (as in the current PR) or add a new test problem?

@hyschive hyschive requested a review from jzuhone July 20, 2024 03:27
@hyschive hyschive assigned jzuhone and hyschive and unassigned hyschive Jul 20, 2024
@hyschive hyschive added enhancement hydro Hydrodynamics test Test problems gpu GPU issues and implementation labels Jul 20, 2024
@jzuhone
Copy link
Collaborator

jzuhone commented Jul 20, 2024

@ShuangShuang0411 @hyschive I plan on reviewing this in detail over the next few days.

@hyschive hyschive mentioned this pull request Jul 20, 2024
Closed
hyschive
hyschive reviewed Jul 24, 2024
View reviewed changes
jzuhone
jzuhone reviewed Jul 26, 2024
View reviewed changes
jzuhone
jzuhone reviewed Jul 26, 2024
View reviewed changes
jzuhone
jzuhone reviewed Jul 26, 2024
View reviewed changes
jzuhone
jzuhone reviewed Jul 26, 2024
View reviewed changes
jzuhone
jzuhone reviewed Jul 26, 2024
View reviewed changes
jzuhone
jzuhone reviewed Jul 26, 2024
View reviewed changes
jzuhone
jzuhone reviewed Jul 26, 2024
View reviewed changes
jzuhone
jzuhone reviewed Jul 26, 2024
View reviewed changes
jzuhone
jzuhone reviewed Jul 26, 2024
View reviewed changes
jzuhone
jzuhone previously requested changes Jul 26, 2024
View reviewed changes
Copy link
Collaborator

@jzuhone jzuhone left a comment

Choose a reason for hiding this comment

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

I left some comments.

ChunYen-Chen
ChunYen-Chen reviewed Sep 19, 2024
View reviewed changes
@jzuhone
Copy link
Collaborator

jzuhone commented Dec 15, 2025

@ChunYen-Chen what's left before we can merge the PR?

@ChunYen-Chen
Copy link
Collaborator

ChunYen-Chen commented Dec 15, 2025

@jzuhone I would like to add the analysis scripts and a README to help reproduce the paper results. If you prefer to merge the code now, I can file a separate PR for the remaining updates.

@jzuhone
Copy link
Collaborator

jzuhone commented Dec 15, 2025

@ChunYen-Chen no, I think it's good for you to include those.

@ChunYen-Chen
Copy link
Collaborator

ChunYen-Chen commented Dec 15, 2025

@jzuhone Could you merge this PR when you have time? This PR includes more scripts, cool-core destruction input files and new initial conditions.

@jzuhone
Copy link
Collaborator

jzuhone commented Dec 15, 2025

@ChunYen-Chen I had a look at it and it looks good, so I merged. Anything else, aside from the conflict that I can fix?

@ChunYen-Chen
Copy link
Collaborator

ChunYen-Chen commented Dec 16, 2025

@jzuhone I think that's all. Thanks for the review.

@jzuhone
Copy link
Collaborator

jzuhone commented Dec 16, 2025

@hyschive I think this is ready for merge!

@hyschive
Copy link
Contributor

hyschive commented Dec 16, 2025

@jzuhone @ChunYen-Chen Thanks for your great work updating and reviewing this PR! Please approve it if you think it’s ready.

Since this is a fairly large PR, with 101 files modified, would you mind if I take another look before merging, at least for the files outside the ClusterMerger test problem? I’m unfortunately fully occupied this month, but I should be able to do this in early January. What do you think?

ChunYen-Chen
ChunYen-Chen approved these changes Dec 16, 2025
View reviewed changes
hyschive
hyschive requested changes Jan 16, 2026
View reviewed changes
Copy link
Contributor

@hyschive hyschive left a comment

Choose a reason for hiding this comment

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

This round of review address several compilation issues introduced by #458. I'll add more comments separately in the coming days.

@hyschive
Copy link
Contributor

hyschive commented Jan 16, 2026

@jzuhone Please check my comment here in case you missed it. Also, could you merge the latest main? Thanks.

I'll add more comments on this PR in the coming days.

hyschive
hyschive requested changes Jan 17, 2026
View reviewed changes
Copy link
Contributor

@hyschive hyschive left a comment

Choose a reason for hiding this comment

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

This round of review addresses additional compilation issues and includes a few minor comments.

hyschive
hyschive requested changes Jan 17, 2026
View reviewed changes
Copy link
Contributor

@hyschive hyschive left a comment

Choose a reason for hiding this comment

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

This round of review focuses on the exact-cooling source files.

hyschive
hyschive requested changes Jan 17, 2026
View reviewed changes
Copy link
Contributor

@hyschive hyschive left a comment

Choose a reason for hiding this comment

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

This round of review focuses on the ExactCooling test problem.

jzuhone and others added 8 commits January 21, 2026 10:05
@jzuhone
Merge branch 'cool-core-public' of ssh://github.com/ShuangShuang0411/…
a422d09 
…gamer-fork into cool-core-public

* 'cool-core-public' of ssh://github.com/ShuangShuang0411/gamer-fork:
  Minor
  Update for CCD simulation
  Add more scripts
  Update for CCD simulations
  fix: prevent use of uninitialized variable
  chore(docs): Sync wiki to doc/wiki [skip-cd]
  Update output file
@jzuhone @hyschive
Apply suggestions from code review
a941989 
Co-authored-by: Hsi-Yu Schive <hyschive@gmail.com>
@jzuhone @hyschive
Apply suggestions from code review
8b86dfa 
Co-authored-by: Hsi-Yu Schive <hyschive@gmail.com>
@jzuhone
Merge branch 'cool-core-public' of ssh://github.com/ShuangShuang0411/…
96f8395 
…gamer-fork into cool-core-public

* 'cool-core-public' of ssh://github.com/ShuangShuang0411/gamer-fork:
  Apply suggestions from code review
  Apply suggestions from code review
@jzuhone
Apply more suggestions from code review
a636e6b
@jzuhone
Merge branch 'main' into cool-core-public
ca69828 
* main:
  check pressure only for the EoSs that need
  check pressure only for the EoSs that need
  add a check for pressure in interpolation
  Default FLU_INT_SCHEME based on SUPPORT_SPECTRAL_INT
@jzuhone @hyschive
Apply suggestions from code review
7977231 
Co-authored-by: Hsi-Yu Schive <hyschive@gmail.com>
@jzuhone @hyschive
Update src/TestProblem/Hydro/ClusterMerger/Init_TestProb_ClusterMerge…
b2373b4 
…r.cpp

Co-authored-by: Hsi-Yu Schive <hyschive@gmail.com>
@jzuhone
Copy link
Collaborator

jzuhone commented Jan 21, 2026 •
edited
Loading

@hyschive I believe I have resolved all of your suggestions and I have merged main into this branch.

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

Reviewers

@hyschive hyschive hyschive requested changes

@jzuhone jzuhone jzuhone approved these changes

@ChunYen-Chen ChunYen-Chen ChunYen-Chen approved these changes

Assignees

@jzuhone jzuhone

@ChunYen-Chen ChunYen-Chen

Labels

enhancement gpu GPU issues and implementation hydro Hydrodynamics test Test problems

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ShuangShuang0411 @hyschive @jzuhone @ChunYen-Chen