Skip to content

[newchem-cpp] transcribe calc_grain_size_increment_species_1d#507

Open
ChristopherBignamini wants to merge 16 commits into
grackle-project:newchem-cppfrom
ChristopherBignamini:newchem-cpp_transcribe_calc_grain_size_increment_species_1d
Open

[newchem-cpp] transcribe calc_grain_size_increment_species_1d#507
ChristopherBignamini wants to merge 16 commits into
grackle-project:newchem-cppfrom
ChristopherBignamini:newchem-cpp_transcribe_calc_grain_size_increment_species_1d

Conversation

@ChristopherBignamini
Copy link
Copy Markdown
Contributor

This PR requires some additional work to fix a numerical issue which occurs in a call with j=k=0 and first iteration of i-loop (j=k=1 in Fortran), when coef3 is equal to zero. I temporarily added a line to skip the cubic equation solution in this case and we need to find a final fix.

@ChristopherBignamini ChristopherBignamini changed the title Newchem cpp transcribe calc grain size increment species 1d [newchem-cpp] transcribe calc_grain_size_increment_species 1d Feb 16, 2026
…transcribe_calc_grain_size_increment_species_1d
@ChristopherBignamini ChristopherBignamini changed the title [newchem-cpp] transcribe calc_grain_size_increment_species 1d [newchem-cpp] transcribe calc_grain_size_increment_species_1d Feb 16, 2026
@ChristopherBignamini
Copy link
Copy Markdown
Contributor Author

pre-commit.ci autofix

@brittonsmith
Copy link
Copy Markdown
Contributor

@ChristopherBignamini, I'm not able to push to your branch to update this PR. Can you merge in the latest changes to newchem-cpp and update this? While you are at it, can you do the same for PR #519 and #520?

…transcribe_calc_grain_size_increment_species_1d
@ChristopherBignamini
Copy link
Copy Markdown
Contributor Author

@brittonsmith sure, done! The failure with Pygrackle test suite is due to the missing gold-standard-nccv8 tag

@brittonsmith
Copy link
Copy Markdown
Contributor

@brittonsmith sure, done! The failure with Pygrackle test suite is due to the missing gold-standard-nccv8 tag

Oops, thought I pushed that. I've done it now and restarted the tests.

@brittonsmith
Copy link
Copy Markdown
Contributor

@ChristopherBignamini, @mabruzzo I've take a closer look at the failed tests and the differences are significant for most of the ionic and molecular species, but even the dust temperature is different from the first iteration. I think we'll need a closer look here.

@ChristopherBignamini
Copy link
Copy Markdown
Contributor Author

@brittonsmith @mabruzzo it could be my fault, there is a TODO in calc_grain_size_increment_species_1d concerning a manual assignment of the first element of drsp for a specific entry of input the indexes: I added that assignment when I was comparing the numerical values of the various steps with the Fortran version and I found a discrepancy due to that first element, most likely because of the different handling of NaN between C++ and Fortran. By manually setting to zero that element in both the C++ and Fortran versions of the subroutine the numerical results were identical. I tried now to remove that assignment and re-run the pygrackle suite: it still fails but I'm not sure the errors are now acceptable. My apologies.

@brittonsmith
Copy link
Copy Markdown
Contributor

@ChristopherBignamini, ok the agreement is much better now, on the level of what I see for PRs #519 and #520. I think this would be acceptable for merging.

Copy link
Copy Markdown
Collaborator

@mabruzzo mabruzzo left a comment

Choose a reason for hiding this comment

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

I started to look this over. I hope to circle back and finish up later today, but I wanted to post what I had so far.

I feel pretty strongly that we should rename alsp_data to kappa_data. You can totally feel free to punt on renaming all of the other variables (maybe open an issue to record some of those renaming ideas?)

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