Skip to content

Conversation

@odiazib
Copy link
Contributor

@odiazib odiazib commented Oct 2, 2024

We've ported the set_hcoeff_vector routine.

This routine utilizes dheff (the effective Henry's Law coefficient), defined here.

Originally, dheff includes definitions for 192 species. In our ported version, we have customized this table to include only 3 gas species currently used in our base case simulation: H2O2, H2SO4, and SO2.

constexpr Real ph = 1.e-5; // measure of the acidity (dimensionless)

const Real t0 = 298.0; // Standard Temperature
const Real ph_inv = 1.0 / ph; // Inverse of PH
Copy link
Contributor

Choose a reason for hiding this comment

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

Both t0 and ph_inv can be constexpr as they are known at compile time.

const Real t0 = 298.0; // Standard Temperature
const Real ph_inv = 1.0 / ph; // Inverse of PH

const Real wrk = (t0 - sfc_temp) / (t0 * sfc_temp);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add an EKAT ASSERT before line 97 to guard against dividing by zero?

  EKAT_KERNEL_ASSERT_MSG((t0 * sfc_temp) !=0,
                         "Error! : set_hcoeff_scalar,"
                         "(t0 * sfc_temp) is zero, division by zero \n");

or something similar....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

t0 is 298, and sfc_temp (surface temperature) is very unlikely to be zero. Thus, I do not think we will encounter a division by zero. In addition, if sfc_temp were to be zero, an error would be produced before any threads reach this point.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that is a good point. I also think it is unlikely to get into divide by zero error here.

@singhbalwinder
Copy link
Contributor

Looks great! I just left some minor comments.

Copy link
Contributor

@mingxuanwupnnl mingxuanwupnnl left a comment

Choose a reason for hiding this comment

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

@odiazib Thank you for porting this subroutine. It looks good to me. I don't have other comments.

@codecov
Copy link

codecov bot commented Oct 2, 2024

Codecov Report

Attention: Patch coverage is 91.89189% with 3 lines in your changes missing coverage. Please review.

Project coverage is 96.14%. Comparing base (89fd234) to head (521ee47).
Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
src/mam4xx/seq_drydep.hpp 91.66% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #353      +/-   ##
==========================================
- Coverage   96.16%   96.14%   -0.02%     
==========================================
  Files          43       44       +1     
  Lines        9490     9526      +36     
==========================================
+ Hits         9126     9159      +33     
- Misses        364      367       +3     
Flag Coverage Δ
?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@odiazib odiazib merged commit 92f1526 into main Oct 3, 2024
8 checks passed
@odiazib odiazib deleted the oscar/seq_drydep_setHCoeff branch October 3, 2024 02:26
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.

4 participants