Skip to content
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

Fix precipitation #418

Merged
merged 3 commits into from
Aug 8, 2023
Merged

Fix precipitation #418

merged 3 commits into from
Aug 8, 2023

Conversation

tommbendall
Copy link
Contributor

This attempts to fix precipitation (issue #334) by:

  • ensuring that the advection term associated with Fallout has its transport label removed
  • adding the prognostic label to that term

It also attempts to resolve some outstanding physics problems by:

  • allowing the Timestepper to take physics schemes
  • the PrescribedTransport time stepper is now only different in allowing the wind to be time-varying

I also implemented a routine to return an equation's active_tracer object for a particular species, which should help with error checking in the physics.

Copy link
Collaborator

@atb1995 atb1995 left a comment

Choose a reason for hiding this comment

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

The changes look sensible to me. My understanding/summary of the changes is:

  • get_active_tracer allows you to get meta data of a field. In the instance of Fallout this is to check if the tracer is a mixing ratio
  • The bug in Fallout is fixed by removing the transport label and adding the labels prognostic and physics. prognostic mean's it is counted in time stepping for physics, whilst removing transport prevents double counting.
  • In the case of PrescribedTransport, a physics time stepper is no longer required. I'm assuming this because having a separate time stepper does not make sense in this case?
  • Timestepper now has physics schemes as well, but with the error thrown if a more complex physics time stepper is used.
    Happy to approve this pull request.

@tommbendall tommbendall merged commit d1ef285 into main Aug 8, 2023
4 checks passed
@tommbendall
Copy link
Contributor Author

Thanks Alex! Yes that's right about the physics not wanting a separate time discretisation for the PrescribedTransport

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.

2 participants