Skip to content

Conversation

@iomaganaris
Copy link
Collaborator

@iomaganaris iomaganaris commented Feb 2, 2026

@iomaganaris iomaganaris requested a review from havogt February 2, 2026 16:29
Comment on lines 106 to 107
x = (zeta * (flx_eff - flx_partial)) / ((1.0 + zeta * vt) * rho) # q update
p = (x * rho * vt + flx_partial) * 0.5 # flux
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 swap the conditionals to avoid the code duplication? I wouldn't like to take this code, because it's clearly less readable.

What's the impact of this change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I reverted this change since I didn't see any significant speed up in the end with my changes

Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to refactor to the following. Do you think it has performance implications?

    if current_level_activated:
        vt = previous_level_q.vc * prefactor * power((rhox_prev + offset), exponent) if previous_level_q.activated else 0.0
        x = (zeta * (flx_eff - flx_partial)) / ((1.0 + zeta * vt) * rho)  # q update
        p = (x * rho * vt + flx_partial) * 0.5  # flux
    else:
        x = q
        p = 0.0

kmin_i: fa.CellKField[bool], # ice minimum level
kmin_s: fa.CellKField[bool], # snow minimum level
kmin_g: fa.CellKField[bool], # graupel minimum level
# original_q: Q,
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO

(moving kmins inside didn't make a difference?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It made the code much slower. I didn't look into why though

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's slightly nicer like this anyway.

@github-actions
Copy link

github-actions bot commented Feb 6, 2026

Mandatory Tests

Please make sure you run these tests via comment before you merge!

  • cscs-ci run default
  • cscs-ci run distributed

Optional Tests

To run benchmarks you can use:

  • cscs-ci run benchmark-bencher

To run tests and benchmarks with the DaCe backend you can use:

  • cscs-ci run dace

To run test levels ignored by the default test suite (mostly simple datatest for static fields computations) you can use:

  • cscs-ci run extra

For more detailed information please look at CI in the EXCLAIM universe.

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