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

Add orographic drag #1233

Merged
merged 1 commit into from
Jan 18, 2023
Merged

Add orographic drag #1233

merged 1 commit into from
Jan 18, 2023

Conversation

jiahe23
Copy link
Contributor

@jiahe23 jiahe23 commented Jan 5, 2023

This PR implements the orographic drag following Garner05.

  • Obtain topo info and regrid to model grid;
  • Implement drag computation;
  • Tests (leave the physical tests to a separate PR);
  • Documentation;
  • Code restructure with recent main updates.

@jiahe23 jiahe23 requested review from LenkaNovak and szy21 January 5, 2023 02:58
@szy21
Copy link
Member

szy21 commented Jan 5, 2023

I may change all orographic_drag to orographic_gravity_wave or orographic_gravity_wave_drag to be clear and consistent with nonorographic_gravity_wave. I'll try to review the code later:)

@charleskawczynski
Copy link
Member

Hi @jiahe23, sorry that #1231 caused rebase conflicts with this PR. I think we can apply the same sort of pattern that is now used in the main branch:

  • Add a orographic_drag CLI flag (done)
  • Add an OrographicDrag type (needed)
  • Define a model getter for OrographicDrag (in src/model_getters.jl)
  • Define orographic_drag_cache(::OrographicDrag, Y) and orographic_drag_cache(::Nothing, Y)
  • Replace orographic_drag && CA.orographic_drag_tendency!(Yₜ, Y, p, t) with CA.orographic_drag_tendency!(Yₜ, Y, p, t, p.atmos.orographic_drag)
  • Define CA.orographic_drag_tendency!(Yₜ, Y, p, t, ::OrographicDrag) and CA.orographic_drag_tendency!(Yₜ, Y, p, t, ::Nothing)

I can help out if it's not clear

@jiahe23
Copy link
Contributor Author

jiahe23 commented Jan 9, 2023

Hi @jiahe23, sorry that #1231 caused rebase conflicts with this PR. I think we can apply the same sort of pattern that is now used in the main branch:

  • Add a orographic_drag CLI flag (done)
  • Add an OrographicDrag type (needed)
  • Define a model getter for OrographicDrag (in src/model_getters.jl)
  • Define orographic_drag_cache(::OrographicDrag, Y) and orographic_drag_cache(::Nothing, Y)
  • Replace orographic_drag && CA.orographic_drag_tendency!(Yₜ, Y, p, t) with CA.orographic_drag_tendency!(Yₜ, Y, p, t, p.atmos.orographic_drag)
  • Define CA.orographic_drag_tendency!(Yₜ, Y, p, t, ::OrographicDrag) and CA.orographic_drag_tendency!(Yₜ, Y, p, t, ::Nothing)

I can help out if it's not clear

Thank you for the guideline, @charleskawczynski . It is very helpful. I'll let you know if I have questions making those changes.

Copy link
Contributor

@LenkaNovak LenkaNovak left a comment

Choose a reason for hiding this comment

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

The Overleaf doc looks good! I will continue the review after Charlie's suggested changes (as discussed offline), but here are a couple of minor things I found earlier.

@jiahe23 jiahe23 force-pushed the jh/OGW branch 4 times, most recently from 44b1563 to 7b109b5 Compare January 14, 2023 17:30
@jiahe23 jiahe23 marked this pull request as ready for review January 14, 2023 17:31
@jiahe23 jiahe23 force-pushed the jh/OGW branch 3 times, most recently from 87e389d to 1a320f4 Compare January 17, 2023 15:46
Copy link
Contributor

@LenkaNovak LenkaNovak left a comment

Choose a reason for hiding this comment

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

Hey Jia, the changes look good! There are some loose functions (or at least I couldn't see where they were being used). It would be good to add unit tests - this will also check you're using all new added functions :). Also, do you have a physical test in mind to check it's producing the correct forcing?

@jiahe23
Copy link
Contributor Author

jiahe23 commented Jan 17, 2023

Hey Jia, the changes look good! There are some loose functions (or at least I couldn't see where they were being used). It would be good to add unit tests - this will also check you're using all new added functions :). Also, do you have a physical test in mind to check it's producing the correct forcing?

Thank, @LenkaNovak ! Good catch from you! The physical test itself seems to be a big project. I have some premature idea and decide to leave it to a separate PR. Let's have a discussion with @szy21 sometime soon.

@jiahe23 jiahe23 force-pushed the jh/OGW branch 4 times, most recently from 47d0df5 to 28124a6 Compare January 18, 2023 00:45
@jiahe23
Copy link
Contributor Author

jiahe23 commented Jan 18, 2023

bors r+

@bors
Copy link
Contributor

bors bot commented Jan 18, 2023

Build succeeded:

@bors bors bot merged commit b265baa into main Jan 18, 2023
@bors bors bot deleted the jh/OGW branch January 18, 2023 18:03
@jiahe23 jiahe23 mentioned this pull request Jan 19, 2023
bors bot added a commit that referenced this pull request Jan 19, 2023
1249: fix mse table r=jiahe23 a=jiahe23

This PR is to fix an error in the mse table introduced by #1233 . 

Co-authored-by: jiahe23 <[email protected]>
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