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

Proposed minimal banding change to prevent fewer than 7 ionization bands #1101

Merged
merged 5 commits into from
Oct 17, 2024

Conversation

jhmatthews
Copy link
Collaborator

@jhmatthews jhmatthews commented Oct 8, 2024

This pull request (PR #1101) introduces updates to the banding strategy and other minor changes. Specifically:

  • Enforcing a minimum of seven bands for ionization, otherwise defaulting to the old 10 hardwired bands we used to use
  • New function ion_bands_init to replace freqs_init and ensure the ionization banding is carried out in a separate function
  • Small adjustments to flags and comments to clarify the behavior of the bands in specific cases.
  • New Function: check_appropriate_banding provides some rudimentary checks on banding, but could be expanded

Testing ongoing.

@jhmatthews
Copy link
Collaborator Author

Regression tests look good for this - I checked that nothing changes when you use TDE and AGN banding with matrix_pow. Things do change when you use TSTAR or CV banding with matrix_pow (as I expected from my changes), but overall the results look better, so I'm merging. The reporting is also helpful, if rudimentary.

@jhmatthews jhmatthews merged commit b1728eb into sirocco-rt:dev Oct 17, 2024
2 checks passed
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.

1 participant