-
Notifications
You must be signed in to change notification settings - Fork 104
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
Allow individual glaciers to merge together #624
Conversation
Hello @matthiasdusch! Thanks for updating the PR.
Comment last updated on December 19, 2018 at 16:19 Hours UTC |
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 a lot for this, this is looking great and I like the approach! This is nice looking code 😍
I have the feeling some parts could be simplified, but I don't think it's necessary for now: if it works, that good! Please double check the docstrings and the part about the possible duplication in code again.
I'm sorry I created a conflict again, hopefully this can be solved online this time. Let me know before you want to merge, I might have another look.
tfl.dx_meter = tfl.map_dx * tfl.dx | ||
|
||
if nr == 0: | ||
# change the array size of tributary main flowline attributs |
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.
When I see this, I wonder if it was easier to instantiate a new Centerline object instead of muting the original one. I'm not as deep into it as you, though, so maybe there are complications I didn't think of
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.
Well I agree mutilating the existing Centerline is not very pretty.
But to initialize a new one, I think we would either have to use some manipulated glacier geometries or copy, adapt and redo the whole centerline processes. Both seems more error prone to me than cutting the line where we know it was initialized for a working glacier.
But I am happy to discuss and change this later on.
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.
I trust your judgement here!
…to growing_glaciers_3
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.
This is a great addition, thanks a lot! Also, very nice code ;-)
I still have a few comments regarding the workflow but feel free to merge as is, we might revisit in the next year (or not), when we freeze v1.1
oggm/tests/test_models.py
Outdated
years = 200 # arbitrary | ||
y0 = 1950 # arbitrary | ||
tbias = -1.0 # arbitrary | ||
mbbias = 0.0 # necessary? |
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.
necessary only if you want the MB at t* , i.e. a balanced MB
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.
When I wrote this, I wasn't sure how to handle the MB calibration bias and have since then forgotten about it....
I now implemented the local_mustar
files to be copied to the merged GDir. MultipleFlowlineMB can now use a different bias for each flowline if they originate from different glaciers.
oggm/workflow.py
Outdated
# preprocess the main glaciers to the end | ||
climate_tasks(gdirs_main) | ||
inversion_tasks(gdirs_main) | ||
execute_entity_task(tasks.init_present_time_glacier, gdirs_main) |
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.
Intuitively I wouldn't (re)do all the preprocessing tasks here. I thought the workflow would be: 1 prepro some glaciers, 2 merge them, 3 run the merge simulation. This task should start at step 2 and assume the glaciers are preprocessed already.
(note that it's also because of my recent developments which allow to start from a pre-processed state, which will become the default workflow for most users)
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, my initial idea was to only preprocess the none relevant glacieres as far as neccessary. Purely with performance in mind.
But I agree, this bloats the workflow function. I changed this. For the unlikely case one runs into performance problems because of this, it's easier to write a dedicated workflow.
oggm/workflow.py
Outdated
# preprocess the tributaries to the end | ||
climate_tasks(gdirs_tribs) | ||
inversion_tasks(gdirs_tribs) | ||
execute_entity_task(tasks.init_present_time_glacier, gdirs_tribs) |
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.
I guess you are doing this to preprocess only the glaciers you really need. I can see why this is useful, but it feels quite redundant.
* Enable glaciers to merge together * removed blank line in test_models.py * added utils.merged_glacier_masks * added workflow.merge_glacier_tasks * added whats_new.rst entry, changed baseline_climate to custom * changed test-function to not download rgi-files * changed initialize_merged_gdir to accept a main geodataframe * changed the workflow function to also preprocess the main glcs * PEP8 issues * copy local_mustar and restructured the workflow function * pytest --verbose on travis script * pytest --verbose in second travis script
This is the same as #600. I made some awful merge/commit errors there. Opening a new PR seemed cleaner and easier.
whats-new.rst
gis.merged_glacier_masks
to provide necessary geometry files for graphicsOGGM glaciers are initialised with current (~2003) glacier geometry and modeled as entities. For all scenarios with glacier retreat or only small advances this is perfectly fine. But for simulations where glaciers advance significantly nearby glaciers might merge together and behave different then when modeled individually.
The first requirement to tackle this issue was solved with #539, as each Flowline got an individual mu*.
This Pull Request will most likely only deal with individual glaciers and their tributary glaciers. Future work could adapt this approach and apply it to regional scales. But I doubt merging glaciers will ever become a fully automated process within all OGGM simulations. More likely merging glaciers will be a deliberate choice for specific simulations.
In this 1. commit I provide two new functions:
intersect_downstream_lines
finds tributary glaciers to a main glacier by intersecting downstream lines. This will separate true tributaries from ones flowing into other valleys.merge_tributary_flowlines
will merge the model_flowlines of the tributary glaciers with the ones of the main glacier. The result is one glacier containing all tributary flowlines.One necessary addition in the process was to add a
Centerline.climatefile
attribute. This allows to use a different climate file for each individual flowline, as tributary glaciers might likely use a different climate reference file than the main glacier one.This PR is still work in progress. I at least want to add a more automatic selection of possible tributary glaciers. But I am happy to take comments or suggestions!
For illustration a simulation with artificial climate around Hintereisferner: For the first run all 5 pictured glaciers (Hintereisferner, two small previously connected pieces, Kesselwandferner in the north and Hochjochferner in the east) are modeled in basic OGGM fashion as entities.
The second plot shows a merged run, where all flowlines of the other 4 glaciers are merged to Hintereisferner.
Note how the upper part of the glaciers are identical in depth and width, but the lower part of the merged glacier is thicker, wider and further advanced as there is now more mass contributing to it.
I think the last two things to do are
gis.merged_glacier_masks
, adapted from the original glacier_masks which provides glacier outlines to the graphics functions. For this I need to merge the outlines of the tributary glaciers and combine them in one shp-file.