search

TheHortonMachine / hortonmachine

The Horton Machine
http://thehortonmachine.github.io/hortonmachine/
GNU General Public License v3.0
43 stars 25 forks source link

Lakes detection removes upstream basins of the lake's downstream basin in the geoframeInputBuilder #68

Closed martindoublem closed 1 year ago

martindoublem commented 3 years ago

In a case where the basin downstream of a lake where to contain more than one upstream basin (meaning more basins than the one containing the lake, in the case there is a confluence in the basin downstream of the lake), the basins upstream are not analyzed and included in the new topology file for the GEOframe input builder

The error seems to come from these lines: https://github.com/TheHortonMachine/hortonmachine/blob/83f980452009ebfdc1ec83767a08d0c5847358ce/modules/src/main/java/org/hortonmachine/modules/GeoframeInputsBuilder.java#L336-L337

It can be seen clearly that the upstream basins are cleared and then the lake basin is added however the other upstream basins are not added again and are therefore ignored from the rest of the analysis. In the case of another lake being upstream, it seems that the lake is not aggregated, and does not cut the basins it is located in.

Example screenshots: screen

Example files: D_basin_issue.zip

As it can be seen in topology and on the map, the basin numbered 391 and 336 in yellow are upstream of the basin number 433, which is itself downstream of the lake (in red). However they (391 and 336) and their upstream basins are excluded from the topology_lakes file (on the left).

It does seem however that the GEOframe input folder were built and populated correctly (in my small example).

moovida commented 3 years ago

Hi @martindoublem , thanks for this very good bug report. I will look into it using your testcase data.

moovida commented 3 years ago

A few comments while I look into it.

you can see that it touches the borders a some basins opening the way for possible single pixels representing mini-basins and that might give results you didn't expect.

Looking at it with a map below: image Maybe you could edit it a bit to better fit in the basin subdivision?

moovida commented 3 years ago

Hi @martindoublem , I think it is fixed. I placed you a result folder in the following archive: geoframe.zip

Could you please check the topology and the results in general to make sure I didn't miss anything?

martindoublem commented 3 years ago

Maybe you could edit it a bit to better fit in the basin subdivision?

I was just using the lakes shapefile of the geoCatalogo, I just selected this one as an example, I had the issue elsewhere as well (such as in Trentino with a different shapefile as well). It seems that whether I look at google maps or the OSM map, this lake shape is very different (if it is even one).

you can see that it touches the borders a some basins opening the way for possible single pixels representing mini-basins and that might give results you didn't expect.

But as I said, to me the problem seems to be independent of the shape of this specific lake. Due to the outlet of the lakes in general, being a point as well as the inlets, the border of the basins, will touch the lake, is this not what I should be doing?

Could you please check the topology and the results in general to make sure I didn't miss anything?

I think you forgot to add the topology file? I see everything else though.

moovida commented 3 years ago

Outch, sorry, I thought the topology file was in the zip. Attaching it here.

tp_lakes.txt

martindoublem commented 3 years ago

Outch, sorry, I thought the topology file was in the zip. Attaching it here.

tp_lakes.txt

Just checked it, looks good to me on this small sample!

If you share, the whole horton machine. I can give it a try on my whole Adige basin!

moovida commented 3 years ago

Ok, do you need a build or just the commit of the fix?

martindoublem commented 3 years ago

Ok, do you need a build or just the commit of the fix?

A build, please, if you don't mind.

moovida commented 3 years ago

Yes, sure, give me a moment.

martindoublem commented 3 years ago

Ok, so I tried it a couple weeks ago and it seemed to solve my issue, I will reopen if necessary.

martindoublem commented 2 years ago

Hi there, would it possible to release the new version of the horton machine that solved this in the releases download package page?

A colleague of mine had the same issue as me today and I could not find a valid download link for the version that corrected this.

Thank you!

moovida commented 2 years ago

We are waiting to include the kriging part we are working on in a future official release. I am not sure: do you need an official release to work with the maven artifacts or do you just need a build?

martindoublem commented 2 years ago

No worries then. I have the build you gave me a while back already. Just wanted to point it out that it was not available yet.

On Tue, Nov 30, 2021, 14:23 Andrea Antonello @.***> wrote:

We are waiting to include the kriging part we are working on in a future official release. I am not sure: do you need an official release to work with the maven artifacts or do you just need a build?

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/TheHortonMachine/hortonmachine/issues/68#issuecomment-982632544, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKCI3F2EVPKDXLFDMK7HEVLUOTF5NANCNFSM5FJ2AOGQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.