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

New Flowmodel added #23

Merged
merged 3 commits into from Feb 3, 2016

Conversation

Projects
None yet
2 participants
@alexjarosch
Copy link
Member

alexjarosch commented Feb 1, 2016

I have added several new things to OGGM:

  1. My MUSCL-SuperBee model is now functional, based on my 2013 paper: A. H. Jarosch, C. G. Schoof, and F. S. Anslow, “Restoring mass conservation to shallow ice flow models over complex terrain,” The Cryosphere, vol. 7, iss. 1, pp. 229-240, 2013.
  2. I have changed the FluxBasedModel to use the modern 2*Aglen/(N+2) instead of the fd parameter
  3. I have changed the KarthausModel to use the modern 2*Aglen/(N+2) instead of the fd parameter
  4. The test_constant_bed case includes now and compares to the MUSCL-SuperBee model as a benchmark.
  5. I have added the test_constant_bed_cliff test case which demonstrates the effect of a bedrock cliff on the three flowline models. This test also highlights the necessity of a proper numerical scheme to deal with such eventualities to avoid spurious mass creation.

alexjarosch added some commits Feb 1, 2016

Added the MUSCL SuperBee flow model and modified the constant bed tes…
…t case to use it. Prepared test case for cliff experiment, however not working yet. Started introducing Aglen to the list of parameters
The cliff bed test case is ready, demonstrates model differences. Agl…
…en is fully integrated in the three flowline models as a parameter in the TestIdealisedCases
@fmaussion

This comment has been minimized.

Copy link
Member

fmaussion commented Feb 1, 2016

Great contribution, thanks a lot! Do not worry about the tests not passing yet, I think I know where it comes from

@@ -300,6 +300,7 @@ def __init__(self, flowlines, mb_model=None, y0=0., fs=None, fd=None):
"""

self.mb = mb_model
self.Aglen = Aglen
self.fd = fd

This comment has been minimized.

Copy link
@fmaussion

fmaussion Feb 1, 2016

Member

To preserve backwards compatibility (at least for a while), setting fd could be equivalent to setting Aglen, and the self.fd property could be removed. Something like:

if fd is not None:
    Aglen = (N+2.) * fd / 2

What do you think?

This comment has been minimized.

Copy link
@alexjarosch

alexjarosch Feb 3, 2016

Author Member

That should be fine, but we should clearly note in the code that this is for legacy backwards compatibility and make an effort to "switch" oggm to the modern notation.

# Done with the loop, prepare output
NewIceThickness = S-B

fl.section = NewIceThickness * width

This comment has been minimized.

Copy link
@fmaussion

fmaussion Feb 1, 2016

Member

Here you could set fl.thick = NewIceThickness

This would have the advantage that you don't have to assume a U-shaped glacier bed. If you have a look at the code for ParabolicFlowline, VerticalWallFlowline, and TrapezoidalFlowline (https://github.com/OGGM/oggm/blob/master/oggm/core/models/flowline.py#L89), you'll see that setting fl.section is equivalent to setting the thickness, which is the actual parameter defining the section. This is how the FluxBasedModel works, regardless of which shape the glacier bed has.

Note that in a way the FluxBasedModel physics are a bit wrong (in https://github.com/OGGM/oggm/blob/master/oggm/core/models/flowline.py#L476, the thickness is physically the same regardless of the bed shape). How "wrong" do you think that your maths above will be if the bed is e.g. parabolic?

I hope that I've been clear, otherwise I'll try to elaborate more.

This comment has been minimized.

Copy link
@alexjarosch

alexjarosch Feb 3, 2016

Author Member

Sounds good, I'll set it to fl.thick = NewIceThickness

added the first round of features from the pull request discussion, a…
…dded legacy fd definition and changed fl.section to fl.thick


# mass balance
m_dot = self.mb.get_mb(fl.surface_h, self.yr)

This comment has been minimized.

Copy link
@alexjarosch

alexjarosch Feb 3, 2016

Author Member

You asked last time round if mb should not be multiplied by glacier width here. I don't know. I took your Karthaus model as a template and there (line 593 above) you don't do that either. Please elaborate on your thought

if fd is not None and Aglen is None:
Aglen = (N+2.) * fd / 2.

self.Aglen = Aglen

This comment has been minimized.

Copy link
@alexjarosch

alexjarosch Feb 3, 2016

Author Member

I have changed this here to calculate Aglen from fd only if fd exists but Aglen not, else we should use Aglen. This works for all the tests I ran, but some of them still return failed. The reason is that your original fd is set to quite a different value.

In your TestIdealisedCases class you set self.fd = 1.9e-24 and I set self.Aglen = 2.4e-24. Converting the fd into the Aglen world you end up with your fd being equivalent to an Aglen of 4.75e-24, which is way softer ice than usually assumed. The perceived softer ice makes the benchmarks fail I guess.

@fmaussion

This comment has been minimized.

Copy link
Member

fmaussion commented Feb 3, 2016

Perfect, thanks! The errors seem to be related to numerical issues: on my system I am able to reproduce the cliff plots but the asserts don't pass. This is not a problem, I'll take care of this. The non-responding tests is because of do_plot=True, which is also OK.

I'll merge this now, take care of my failing tests in a separate PR and come back to you soon about this width stuff, ok?

fmaussion added a commit that referenced this pull request Feb 3, 2016

@fmaussion fmaussion merged commit ccfc8e7 into OGGM:master Feb 3, 2016

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build cancelled
Details
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
@fmaussion

This comment has been minimized.

Copy link
Member

fmaussion commented Feb 3, 2016

OK, just fixed the tests here: #23

I'll come back to you in the next couple of days. Thanks again!

@alexjarosch

This comment has been minimized.

Copy link
Member Author

alexjarosch commented Feb 5, 2016

Good stuff, looking forward to hear from you.

@alexjarosch alexjarosch deleted the alexjarosch:flowmodel-dev branch Feb 5, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.