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

Coriolis selector and new schemes #212

Merged
merged 9 commits into from
Jun 11, 2019

Conversation

jm-c
Copy link
Member

@jm-c jm-c commented Mar 7, 2019

What changes does this PR introduce?

  • add 2 new Coriolis schemes for vector-invariant formulation.
  • add new (run-time) coriolis-scheme selector "selectCoriScheme"
    to replace (in src code) "useJamartWetPoints" and "useEnergyConservingCoriolis";

What is the current behaviour?

  • multiple coriolis-scheme selector.

What is the new behaviour

  • single coriolis-scheme selector (selectCoriScheme); keep full
    backward compatibility ; And add 2 new scheme for V.I.

Does this PR introduce a breaking change?

no.

Other information:

new scheme: selectCoriScheme=2 seems to conserve Angular-Momentum better
than other scheme.

Suggested addition to tag-index

o pkg/mom_vecinv:

  • add 2 Coriolis schemes which satisfy: d/dt U = f V ; d/dt V = -f U
    (with U,V = vertically integrated flow).
  • add new run-time coriolis-scheme selector "selectCoriScheme" to replace
    "useJamartWetPoints" and "useEnergyConservingCoriolis"; but keep these 2
    in namelist (backward compatible) to set "selectCoriScheme" accordingly.

jm-c and others added 4 commits March 7, 2019 17:35
- add new (run-time) coriolis-scheme selector "selectCoriScheme"
  to replace (in src code) "useJamartWetPoints" and "useEnergyConservingCoriolis";
  keep these 2 params in namelist to set "selectCoriScheme" accordingly.
- add 2 new Coriolis schemes for Vector-Invariant formulation; each satisfies:
  Vertical integral of Coriolis equal Coriolis applied to Vert.Integrated flow
  (while no current other scheme does).
@jm-c
Copy link
Member Author

jm-c commented Mar 9, 2019

@jrscott : I trie to update the parameter table. Please check when you have time. Also, I choose a different
way to describle multiple choice, using "=2 : description ; =3 : description ...." instead of "2= description ; 3= description ...." I feel this new way more clear. But I did not change the few other multiple-choice params description. Something to decide.

@jrscott
Copy link
Member

jrscott commented Mar 11, 2019

@jm-c I'll take a look and we can discuss this week

@jrscott
Copy link
Member

jrscott commented Mar 14, 2019

@jm-c, I think we need to make some changes here. Between the ch 2 and ch 3 edits, was confused. Had the following questions:

  • is the energy conserving form better? If so, might it be worth making this the default?
    ("for historical reasons" seems a poor explanation of defaulting to an inferior discretization...
    I realize this involves some work, perhaps can be grouped with another change affecting results)

  • Unless I'm missing something, there seems no reason to keep the deprecated parameters in the manual (either the mentions in ch 2 or 3). We keep them in the namelists (in the code) but remove all traces in the documentation; this was done for a host of other deprecated parameters.

  • I prefer the way I listed the options however I think I should replace my comma delimiter with a semi-colon, and I think it would be improved. I found using both the colon and semicolon confusing in regards to what grouped with what. Let's decide on this and I can change throughout.

@jrscott
Copy link
Member

jrscott commented Mar 14, 2019

Perhaps the better plan is to make a table for the relevant section in ch 2 (similar idea as done for table 2.1) that explains these choices in greater detail, and refer to the table in the ch 3 parameter list table.
If you can explain the choices to me so I understand them better, I can do this :)

@christophernhill
Copy link
Member

@jrscott to work with @jm-c on adding issue for the need for further tests beyond flat bottom atmos. Will also update PR to reference test experiment in verification_other which is WIP ( MITgcm/verification_other#2. ) pending this code merge

@jm-c
Copy link
Member Author

jm-c commented Apr 10, 2019

Plan is to switch new experiment (in verification_other, atm_strato) to use one of the 2 new Coriolis scheme (selectCoriScheme=2). One of the advantage of this new scheme is to conserve Angular Momentum
(without topography) and this new aquaplanet-type experiment is good candidate to test it.

@christophernhill
Copy link
Member

@jrscott to finish docs

@christophernhill looked at code. LGTM

…_and_scheme

fixed conflict
removed useJamartWetPoints and useEnergyConservingCoriolis from parm table doc
as they have been replaced in fn by selectCoriScheme (although remain in code)
@jrscott
Copy link
Member

jrscott commented May 17, 2019

@jm-c I think (?) I brought this PR up to date and properly fixed the merge conflict in the parameter table (I did delete the two deprecated parms from the table too). Ball in @edoddridge's court now to point out what I did wrong :) But don't push the green button just yet, let me look at the ch 2 doc section.

@edoddridge
Copy link
Member

Sure thing. I'll look at it this week, but won't merge it.

@jm-c
Copy link
Member Author

jm-c commented May 22, 2019

@jrscott took a quick look at changes in section 3 tables. Looks good.
Since this PR code changes has been approved (as it) for some time now (since Apr 11), could we speed up a bit section 2 review to get closer to the merging point ?

@edoddridge
Copy link
Member

I can't see any problems with your merge. 👍

removed mention of deprecated parm, and more explicit what eq used in energy-cons form
@jrscott
Copy link
Member

jrscott commented May 24, 2019

@jm-c please check this last change for accuracy. I removed the mention of the deprecrated parm. I basically did as we discussed, I believe, despite this section needing further attention.

When this is checked in -- I will create an issue that this section (and the VI:Coriolis section) need pretty serious overhaul, mention of Jamart etc.

"option 2" is used when run-time integer parameter selectCoriScheme is set to two
@jm-c
Copy link
Member Author

jm-c commented Jun 10, 2019

Would like to merge this soon (may be tomorrow). No major changes to the code for quite a long time

@jm-c jm-c merged commit 7843dde into MITgcm:master Jun 11, 2019
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

4 participants