-
Notifications
You must be signed in to change notification settings - Fork 4
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
Design Document: Omega-0 algorithm formulation #33
Design Document: Omega-0 algorithm formulation #33
Conversation
@mark-petersen, as I mentioned in #32, please keep the checklist when you make new Omega PRs. |
This PR is still under development. Early comments are welcome, but I will add reviewers and post again when the algorithm section is complete. |
3e3776a
to
27be3fd
Compare
@mark-petersen, it doesn't look like you rebased onto the current |
27be3fd
to
5286c80
Compare
Move constants from ice_constants_colpkg.F90 to mpas_seaice_constants.F
e69b893
to
a670ea0
Compare
30ba0c0
to
69a0915
Compare
663d824
to
62cec08
Compare
During this review, the documentation is available here for convenient viewing. |
1bc24ad
to
6aab934
Compare
Rebased and squashed. This is document is now done and ready for review. |
Please review when you get a chance: @sbrus89 @mwarusz @hyungyukang I would like to make any revisions and merge this week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mark-petersen, this looks great to me. I just had a couple really minor comments from reading through.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good @mark-petersen. I also only have minor comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mark-petersen , changes look great. @sbrus89 and @mwarusz 's reviews covered my suggestions and improved the doc. Many thanks for your work!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing those changes, @mark-petersen. This is a really nice resource to have!
982f4dd
to
1a888d6
Compare
@mwarusz I corrected based on your comments. The full document is here. In particular, I expanded the section on the perpendicular gradient and the section on del2. Please review again if you like, and I can merge in once you've approved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mark-petersen I found more minor things.
@mwarusz, thank you for your detailed review. I addressed all of your comments, and the revised document is here. |
@mark-petersen this is really nice. I think it is so great to have this all in one place. Thanks for all the work on it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mark-petersen Thanks for addressing all of my previous comments. This looks almost ready, but I do have some new suggestions.
$$ | ||
The normalization can be found through simple geometric arguments to be similar to above, | ||
$$ | ||
A_v = \sum_{e\in EV(v)} \frac{{\bar A}_{e}}{4}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this simply be {\hat A}_v
? I don't see this expression in MPAS-O code. It just uses areaTriangle
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I updated it to {\hat A}_v
. I just meant to show here the equivalence of the two sides in area. I updated the words to make that more clear.
@mwarusz thanks for your attention to detail, and sorry for the delay. The revised documentation is available here. I added a new figure to show exactly what each area region is to help future readers, below the variable definitions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mark-petersen for your hard work on this document. The added figure is very helpful. The only issue I found are two typos where c
is used as cell index instead of i
, so I think I can safely approve.
df80916
to
5b38158
Compare
rebased to handle conflicts. |
Add section 3, the algorithm formulation, to the design document. During this review, the documentation is available here for convenient viewing.
Checklist