-
Notifications
You must be signed in to change notification settings - Fork 109
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
Water terminating #1403
base: master
Are you sure you want to change the base?
Water terminating #1403
Conversation
Hello @jmalles! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2022-08-14 13:23:21 UTC |
Thanks @jmalles ! This looks like a great piece of work. I'll have a look next week. |
OK so I had a quick look at all the changes - in my view this looks great, but of course there is still some work before merge ;-) I think that something that would be very useful as a first step is to make sure that all the tests unrelated to calving pass. The calving tests will need to be adapted for sure, also including inversion. Do you think you can manage to do that @jmalles ? If you need help, it's worth discussing if this could become a team effort with @anoukvlug as this is quite an important improvement to OGGM. |
I would be happy to contribute. So if there is anything I can help with, just send me a message :) |
I will definitely try, but am not quite sure yet when I will be able to finish that, since I will be in North America until end of June. I will get in contact with @anoukvlug soon though! |
@jmalles is this the version of the code which was used for the paper? Is it worth that I and @pat-schmitt have a look at it over the christmas break or did you want to change things? |
Yes, I think this should be the version used for the paper and I was not planning to change things at the moment. Though I think the geodetic calibration function with using Will's data needs some more work, as it is kind of hardcoded right now... Other than that, I hope it is in a shape that is worth having a look at. |
@jmalles I changed my mind with respect to what I said on Slack: please keep this PR unchanged and open two new, fresh PRs. Lets start with one only adding the parameterisation (not the calibration). It should be possible to add the parameterization and test it without the rest. Please remove bits of code which have become obsolete (e.g. the diagnostic variables you did not use) because we can retriev them later from this PR. Please add some inline comments where it might be necessary. If we work together on smaller bits we can get there! |
* Fix issue with TANDEM DEM and minor RGI7 issue * Test fix
* Attempt fix docs * pin scipy * add package version * deprec * kuddos
* Add UTM coordinate system * Revert defaults
Bumps [docker/build-push-action](https://github.com/docker/build-push-action) from 3 to 4. - [Release notes](https://github.com/docker/build-push-action/releases) - [Commits](docker/build-push-action@v3...v4) --- updated-dependencies: - dependency-name: docker/build-push-action dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Leaving this open for documentation for a while - but fixing this will require a full set of new code to deal with the specifics of v1.6 |
My code for inverting and running water-/marine-terminating glaciers.