Skip to content

Feature/second-order-runge #277

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

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Conversation

roger-teoh
Copy link
Collaborator

Closes #XX

Changes

  • Updates CoCiP time integration step from the first-order Euler approach to the second-order Runge-Kutta scheme. This fixes the issue of contrail energy forcing convergence, and makes its implementation consistent with CoCiP Fortran.

Breaking changes

Features

Fixes

Internals

Tests

  • QC test passes locally (make test)
  • CI tests pass

Reviewer

Select @ reviewer

@roger-teoh
Copy link
Collaborator Author

@zebengberg We should probably set the default CoCiP parameters to radiative_heating_effects = True, and second_order_runge = True

@mlshapiro
Copy link
Contributor

@roger-teoh I have another issue out there #199 -- I think it would be good to change all the default parameters at once in the next release!

In particular, I think we should set the max_age in line with your recent paper.

Copy link
Contributor

@zebengberg zebengberg left a comment

Choose a reason for hiding this comment

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

Thanks Roger -- this looks great! I made a few small cosmetic suggestions.

Comment on lines 224 to 228
radiative_heating_effects: bool = False
radiative_heating_effects: bool = True
Copy link
Contributor

Choose a reason for hiding this comment

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

Per Marc's comment, let's change this back to False here, then we'll update in a separate PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Resolved. I have also set second_order_runge to False. We can update in a separate PR.

Comment on lines 174 to 175
#: CoCiP time integration using the second-order Runge-Kutta scheme
#: Up until v0.54.3, the time integration step was implemented as a First-order Euler method,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#: CoCiP time integration using the second-order Runge-Kutta scheme
#: Up until v0.54.3, the time integration step was implemented as a First-order Euler method,
#: Perform CoCiP time integration using the second-order Runge-Kutta scheme.
#: If False, a first-order Euler method is used instead. A first-order Euler's method
#: was used exclusively in earlier versions of pycontrails.
#:
#: .. versionadded:: 0.54.4

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Resolved

# Second-order Runge-Kutta scheme
if params["second_order_runge"]:
# `contrail_2` calculated in first-order Euler method is now used as intermediate values
contrail_12 = copy.deepcopy(contrail_2)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
contrail_12 = copy.deepcopy(contrail_2)
contrail_12 = contrail_2.copy()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Resolved.

details, see :func:`time_integration_runge_kutta`.
"""
# First-order Euler method
contrail_12 = copy.deepcopy(contrail_1) # Set intermediate values to the initial values
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
contrail_12 = copy.deepcopy(contrail_1) # Set intermediate values to the initial values
contrail_12 = contrail_1.copy() # Set intermediate values to the initial values

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Resolved

@zebengberg zebengberg force-pushed the feature/second-order-runge branch from 023bcf9 to 50879e6 Compare November 27, 2024 21:10
@zebengberg zebengberg force-pushed the feature/second-order-runge branch from 7471d2a to b63967a Compare February 4, 2025 16:12
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