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

Adds Zonal Diffusion #179

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

Adds Zonal Diffusion #179

wants to merge 10 commits into from

Conversation

HenryDane
Copy link
Contributor

This pull request adds zonal advection-diffusion, zonal heat diffusion, and zonal moist diffusion. Additionally, this makes the adv-diff solver support periodic boundary conditions.

I've also added some unit tests to verify that the changes work.

Please let me know if you have any comments or feedback or questions.

@brian-rose
Copy link
Collaborator

Hi @HenryDane, this looks awesome! Thanks for the contribution, and especially for including tests with your PR! It may take me a bit of time to digest everything you've done here.

climlab/tests/test_advdiff_procs.py Outdated Show resolved Hide resolved
Fixes missing import for `climlab.const` as per Brian Rose's recommendation.

Co-authored-by: Brian Rose <brose@albany.edu>
@HenryDane
Copy link
Contributor Author

I committed the change you suggested -- thanks very much for pointing that out to me. It looks like all the tests are passing now.

@brian-rose
Copy link
Collaborator

I committed the change you suggested -- thanks very much for pointing that out to me. It looks like all the tests are passing now.

Great! I'll take a closer look at the code when I get a chance.

@brian-rose brian-rose self-requested a review May 17, 2023 21:01
@HenryDane
Copy link
Contributor Author

I've been adding commits here (f256da through 4a6ba5b) to fix an issue I noticed with this new version where if a model had both zonal and meridional adv-diff processes, the fluxes (diffusive_flux, total_flux, advective_flux, flux_convergence) for one adv-diff process could get over-written by others.

These new commits fix this problem by adding an optional diag_suffix so that when a process inherits from AdvectionDiffusion, it can declare a suffix for the aforementioned attributes, thereby fixing the issue.

With these changes, I believe that zonal advection-diffusion, zonal heat diffusion, and zonal moist diffusion should now work correctly both on their own and in the presence of other adv-diff processes.

@brian-rose
Copy link
Collaborator

@HenryDane can you merge in the latest changes from main branch into this PR? That will clear up the unrelated test failures.

Merge climlab/main into diff_lon
@HenryDane
Copy link
Contributor Author

Yes, of course. I just applied the merge and all the tests seem to be passing now.

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.

None yet

2 participants