Skip to content

[newchem-cpp] Transcribe calc_gr_balance_g#519

Merged
brittonsmith merged 24 commits into
grackle-project:newchem-cppfrom
ChristopherBignamini:newchem-cpp_transcribe_calc_gr_balance_g
Jun 11, 2026
Merged

[newchem-cpp] Transcribe calc_gr_balance_g#519
brittonsmith merged 24 commits into
grackle-project:newchem-cppfrom
ChristopherBignamini:newchem-cpp_transcribe_calc_gr_balance_g

Conversation

@ChristopherBignamini

@ChristopherBignamini ChristopherBignamini commented Mar 24, 2026

Copy link
Copy Markdown
Contributor

This PR depends on #516. Cleanup needed after analysis of numerical discrepancies.

@ChristopherBignamini ChristopherBignamini changed the title Newchem cpp transcribe calc gr balance g [newchem-cpp] Transcribe calc_gr_balance_g Mar 24, 2026
@ChristopherBignamini

Copy link
Copy Markdown
Contributor Author

pre-commit.ci autofix

@ChristopherBignamini

Copy link
Copy Markdown
Contributor Author

pre-commit.ci autofix

@ChristopherBignamini

Copy link
Copy Markdown
Contributor Author

pre-commit.ci autofix

@brittonsmith

Copy link
Copy Markdown
Contributor

@ChristopherBignamini, do you think you could fix these merger conflicts?

@ChristopherBignamini

Copy link
Copy Markdown
Contributor Author

@brittonsmith @mabruzzo should I remove the _g suffix in calc_gr_balance_g?

@brittonsmith

Copy link
Copy Markdown
Contributor

@ChristopherBignamini @mabruzzo, I've looked at the test failures here and they are acceptable differences in my opinion. It is only the free-fall models with dust that show differences and they are all of the order that we have accepted previously. I'm not sure this has been reviewed yet, but from a testing standpoint this can be merged.

@mabruzzo mabruzzo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The comment I put in #507 applies here

Comment on lines +218 to +231
// FORTRAN_NAME(calc_grain_size_increment_species_1d)(
// &my_chemistry->grain_growth, itmask,
// &inject_pathway_props->n_pathways, &my_fields->grid_dimension[0],
// &my_fields->grid_dimension[1], &my_fields->grid_dimension[2],
// &idx_range.i_start, &idx_range.i_end, &idx_range.jp1, &idx_range.kp1,
// &dom, my_fields->density, &n_selected_inj_paths, grsp_density,
// repacked_inj_path_metal_densities.data(), repacked_yields.data(),
// repacked_size_moments.data(), &bulk_density,
// internal_dust_prop_buf.grain_sigma_per_gas_mass.data[grsp_i],
// internal_dust_prop_buf.grain_dyntab_kappa.data[grsp_i], gr_N,
// &gr_Size,
// &inject_pathway_props->log10Tdust_interp_props.parameter_spacing[0],
// inject_pathway_props->log10Tdust_interp_props.parameters[0],
// repacked_opac_table.data());

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We can remove this now

Suggested change
// FORTRAN_NAME(calc_grain_size_increment_species_1d)(
// &my_chemistry->grain_growth, itmask,
// &inject_pathway_props->n_pathways, &my_fields->grid_dimension[0],
// &my_fields->grid_dimension[1], &my_fields->grid_dimension[2],
// &idx_range.i_start, &idx_range.i_end, &idx_range.jp1, &idx_range.kp1,
// &dom, my_fields->density, &n_selected_inj_paths, grsp_density,
// repacked_inj_path_metal_densities.data(), repacked_yields.data(),
// repacked_size_moments.data(), &bulk_density,
// internal_dust_prop_buf.grain_sigma_per_gas_mass.data[grsp_i],
// internal_dust_prop_buf.grain_dyntab_kappa.data[grsp_i], gr_N,
// &gr_Size,
// &inject_pathway_props->log10Tdust_interp_props.parameter_spacing[0],
// inject_pathway_props->log10Tdust_interp_props.parameters[0],
// repacked_opac_table.data());

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment thread src/clib/calc_grain_size_increment_1d.F Outdated
/// @param[in] SN_kp0sp_data Tables of values for opacity calculations
///
/// @par History
/// modified: February, 2026 by Christopher Bignamini & Matthew Abruzzo; C++

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// modified: February, 2026 by Christopher Bignamini & Matthew Abruzzo; C++
/// modified: February, 2026 by Christopher Bignamini; port to C++

@ChristopherBignamini ChristopherBignamini May 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@mabruzzo I’m just doing the dirty work here — without the transcription tool you developed, this would have been infinitely harder. 😄
Partially done in #507

/// @todo
/// The subroutine's name should be more descriptive
///
/// @param[in] igrgr Flag to solve grain growth reactions ()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// @param[in] igrgr Flag to solve grain growth reactions ()
/// @param[in] igrgr Flag to solve grain growth reactions

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in #507

/// port
void calc_grain_size_increment_species_1d(
int igrgr, const gr_mask_type* itmask, int SN0_N, int in, int jn, int kn,
IndexRange idx_range, gr_float* density_data, int nSN,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can we convert gr_float* density_data to const gr_float* density_data?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in #507

/// @param[in] itmask Specifies the general iteration-mask of the @p idx_range
/// for this calculation.
/// @param[in] SN0_N Number of modelled injection pathways
/// @param[in] in, jn, kn Dimensions of the computational grid

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

(I think) it would be great if we could replace in, jn, and kn with a single 3-element array arg called grid_dimensions.

But, I'm totally happy for other feedback

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in #507

/// @param[in] in, jn, kn Dimensions of the computational grid
/// @param[in] idx_range Specifies the current index-range
/// @param[in] density_data Pointer to the density field data
/// @param[in] nSN Number of selected injection pathways

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It would be great if we could replace nSN with n_selected_inj_paths

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in #507

Comment on lines +41 to +42
/// @param[in] SN_metal_data Pointer to the field data for the metal densities
/// for the current injection pathway

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It would be great if we could rename SN_metal_data to selected_inj_path_metal_densities.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in #507

Comment on lines +41 to +42
/// @param[in] SN_metal_data Pointer to the field data for the metal densities
/// for the current injection pathway

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// @param[in] SN_metal_data Pointer to the field data for the metal densities
/// for the current injection pathway
/// @param[in] SN_metal_data Pointer to repacked selected injection pathway
/// metal densities for the current @p idx_range. This is a 2d array where
/// the contiguous axis holds @p in elements and the other axis has space
/// for @p SN0_N entries (in practice only the first @p nSN indices along
/// this axis are used).

(Obviously this needs to be updated if you choose to update SN_metal_density, SN0_N or nSN.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in #507

Comment thread src/clib/dust/calc_grain_size_increment_species_1d.hpp Outdated
@ChristopherBignamini

Copy link
Copy Markdown
Contributor Author

pre-commit.ci autofix

@mabruzzo mabruzzo changed the base branch from newchem-cpp to main June 11, 2026 04:40
@mabruzzo mabruzzo changed the base branch from main to newchem-cpp June 11, 2026 04:40
@mabruzzo

Copy link
Copy Markdown
Collaborator

At a glance this looks good to me. Would you mind merging newchem-cpp into this branch just to make the diff easier to understand?

@ChristopherBignamini

Copy link
Copy Markdown
Contributor Author

Hi @mabruzzo I could be wrong but my git says that this branch is already up-to-date with newchem-cpp (of the original repo)

@brittonsmith brittonsmith changed the base branch from newchem-cpp to main June 11, 2026 18:28
@brittonsmith brittonsmith changed the base branch from main to newchem-cpp June 11, 2026 18:28
@brittonsmith brittonsmith merged commit 09b0763 into grackle-project:newchem-cpp Jun 11, 2026
3 of 6 checks passed
brittonsmith added a commit that referenced this pull request Jun 11, 2026
brittonsmith added a commit that referenced this pull request Jun 11, 2026
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.

3 participants