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

Changes in calving functions to fix water depth #720

Merged
merged 12 commits into from Apr 3, 2019

Conversation

Projects
None yet
3 participants
@bearecinos
Copy link
Contributor

commented Mar 21, 2019

Hello,
I have modified both calving functions so they can cope with a fix water depth from Bathymetry data.
Let me know if you are happy with the changes.

Cheers
Bea

@pep8speaks

This comment has been minimized.

Copy link

commented Mar 21, 2019

Hello @bearecinos! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-04-03 15:25:00 UTC
@fmaussion

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

Hi Bea, for some reason your PR deleted all empty lines, which is not good for the docstrings... Can you start from a fresh branch again?

@TimoRoth TimoRoth force-pushed the bearecinos:master branch from d0ce57b to a1e588d Mar 25, 2019

@bearecinos

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

sorry about that just asked timo for help ... and he fixed very fast 😄 . Thank you @TimoRoth

@fmaussion

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

One good way to avoid all this is to use branches instead of working on master. for next time ;-)

@TimoRoth TimoRoth force-pushed the bearecinos:master branch from a1e588d to 0ea2e20 Mar 25, 2019

@fmaussion
Copy link
Member

left a comment

I'm leaving this open for now, as I'm not yet too sure to understand what we are doing - given that this is in the paper we will have to merge anyway, but I'll wait until I read the paper.

out = calving_flux_from_depth(gdir,
water_depth=water_depth,
thick=thick,
fix_water_depth=True)

This comment has been minimized.

Copy link
@fmaussion

fmaussion Mar 29, 2019

Member

I don't understand this part. If we fix the water depth, shouldn't the original thickness be water_depth + 1? Or do we allow negative free-board? In any case, I don't think that t_altitude should play any role here, because this is what becomes a free parameter in the new loop.

This comment has been minimized.

Copy link
@bearecinos

bearecinos Apr 1, 2019

Author Contributor

Hello sorry for late reply ... took sort of Friday afternoon off...
I just checked and it does not change any results but you are right. This should be water_depth + 1m.
I will also mention this on the Discussion section.

bearecinos and others added some commits Apr 1, 2019

@fmaussion

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

@bearecinos I made some modifs to your code, the only significant changes are:

  • the varying free board is now correctly computed and written out
  • the water_depth kwarg is now called initial_water_depth
  • the initial_water_depth now defaults to 1/3 of the terminus elevation, clipped to a min of 10m

can you review these changes? Then you will have to re-run everything with the modifs I'm afraid...

Ideally your code (i.e. outside of OGGM) should be as minimal as possible: just setting up the run and analyze the results - does this PR help in this respect?

@bearecinos

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

Yes I will read all this and get back to you... First I will re run everything. If anything comes up I will let you know. It might take me more than a few days.

Merge pull request #19 from OGGM/master
Changes in calving function
@fmaussion

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

It might take me more than a few days.

really? Then don't do it.

@fmaussion

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

OK Bea, I've implemented the changes we talked about yesterday. No external changes, just internal:

  • moved the find_calving task to the inversion module in order to make a real task out of it (maybe you have to rename things in your sript but a find/replace should be enough)
  • changed the internals of the glacier_statistics task, but this should not change anything in your code. (I also added more interesting output)
@bearecinos

This comment has been minimized.

Copy link
Contributor Author

commented Apr 3, 2019

Thanks I will implement this and let you know if I don't understand something.

fmaussion added some commits Apr 3, 2019

@fmaussion fmaussion merged commit ece3387 into OGGM:master Apr 3, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.05%) to 87.638%
Details
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.