Riverscapes / pyBRAT

pyBRAT - Beaver Restoration Assessment Tool (Python)
http://brat.riverscapes.xyz
GNU General Public License v3.0
10 stars 10 forks source link

Segment network by Roads #66

Closed joewheaton closed 6 years ago

joewheaton commented 6 years ago

While in the field in Sierras, we noticed we were doing Data Capture for verification very often from bridges and road crossings. We found ourselves doing separate validations for upstream and downstream. Therefore, we discussed it would be nice to add an optional tool to workflow for further segmenting the network input to BRAT based on the input of a polyline road network layer. This would:

  1. Require an input of a vector polyline layer and ask user to specify type (i.e. Road, Railroad, etc.)
  2. Produce a new ouptut (similar to #55) of a stream network input to BRAT. Add a tag in project file to this network to say that it had been segmented by'road's or 'railroads'

This is an optional command, but is nice for ending up with reaches that are broken at boundaries we typically view from.

It might also be nice to output 'nodes' that represent these intersections as a point file intermediate. These could later be attributed as bridges, culverts, fords, etc. or unknown in a CrossingType field. This might be helpful context in the risk layers?

banderson1618 commented 6 years ago

That should be pretty quick to do. I'd say I can do it in a day, maybe up to two if there's unexpected problems.

I can split the stream network by a given polyline input, and I can create points for intersections (I think, based on some quick research I did just now), but I don't know a good way to identify a value for CrossingType. Maybe we allow the user to give a default value for CrossingType, and then they can edit the output if they need to after?

banderson1618 commented 6 years ago

I'm low on tasks right now, so I'm taking a look at this issue right now.

My thinking is that this would be best implemented as a checkbox in the BRAT Table tool, something like "Check to segment network by roads". @wally-mac, @bangen, @joewheaton, does this sound good to you? I won't push any changes until I get feedback on this, but I'll start implementing it on my local repository.

wally-mac commented 6 years ago

Yes, I like this idea of a check box, I think the same approach could be used for the land ownership.

joewheaton commented 6 years ago

Agreed on checkbox. @wally-mac can you assign someone other than @banderson1618 to test this and report their findings. Then close if its working like we want...

wally-mac commented 6 years ago

@Albonicomt next time you run BRAT can you please test this out (see above) and report your findings? Thanks

banderson1618 commented 6 years ago

@Albonicomt did you ever get around to testing this option? It'd be good to close this issue if we could.

Albonicomt commented 6 years ago

Braden, I have not yet rested this out.

On Mon, Jun 25, 2018, 10:36 AM Braden Anderson notifications@github.com wrote:

@Albonicomt https://github.com/Albonicomt did you ever get around to testing this option? It'd be good to close this issue if we could.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Riverscapes/pyBRAT/issues/66#issuecomment-400015924, or mute the thread https://github.com/notifications/unsubscribe-auth/AlWryEqPVIJWaxqiSXsIyCsuETD0ZDzxks5uARGagaJpZM4UPEln .

bangen commented 6 years ago

@banderson1618 I'm wondering how segmenting by roads within the BRAT table tool will effect results from the drainage area check tool? The latter assumes that each reach has a distance DS field that's correctly populated. We're currently creating/attributing this field when segmenting the network. So, are you updating those distance fields? Just want to make sure you validated/tested before posting the code. Thanks!

banderson1618 commented 6 years ago

I hadn't thought about that, @bangen, thanks for bringing that up.

I'm assuming that both streams will inherit the distance value from the original stream, so they would be "processed" at the same time. The worst case scenario would be the segment further upstream getting the value of the downstream segment. Because it would only be one stream segment down, I don't think this would be a very dramatic effect, and it would only happen 50% of the time. Still, we should probably try to avoid that. I'll get on that.

banderson1618 commented 6 years ago

I found something odd: the segments that have been split apart often have the same DA value, even before the DA check tool is run. I'm not sure why that is. I've checked, and the buffers seem to be created properly, so that's not the problem. I'll have to look into this more carefully tomorrow, and see if it happens with segments besides the ones that are split by the segment option.

bangen commented 6 years ago

@banderson1618 Can you please send me the dataset you were testing?

banderson1618 commented 6 years ago

@bangen, I just sent you an email.

bangen commented 6 years ago

@banderson1618 I took a look at the data you sent. The issue is with this code block in the BRAT table tool.

if should_segment_network:
    segment_by_roads(seg_network, seg_network_copy, road)
else:
    arcpy.CopyFeatures_management(seg_network, seg_network_copy)

# --check input network fields--
# add flowline reach id field ('ReachID') if it doens't already exist
# this field allows for more for more 'stable' joining
fields = [f.name for f in arcpy.ListFields(seg_network_copy)]
if 'ReachID' not in fields:
    arcpy.AddField_management(seg_network_copy, 'ReachID', 'SHORT')
    with arcpy.da.UpdateCursor(seg_network_copy, ['FID', 'ReachID']) as cursor:
        for row in cursor:
            row[1] = row[0]
            cursor.updateRow(row)

The 'ReachID' isn't updated after segmenting since it already exists. As a result you ended up with multiple segments and buffers with the same reach id number. Since our dictionary joining is done by 'ReachID', multiple reaches ended up with the same DA value. Also, the field type should be changed from 'SHORT' to 'LONG'. If you could update that would be great.

As a general practice we should do more testing/validating of changes before pushing. I know this can be a hassle and I don't expect we'll always catch all issues. But if we can circumvent some of them beforehand that will save time having to re-run basins through the entire model. If this would be easier to handle by setting up a development branch feel free to go ahead and set one up. Thanks!

banderson1618 commented 6 years ago

That makes sense, good catch! I think it's fair to say that I didn't do my due diligence when considering how the segmenting would impact the larger operating of the tool, so I appreciate this reminder. I'll push up a fix ASAP.

On the subject of testing, it might be a good idea to have an optional function called "validateOutput()", which would be ran at the end of the tool. Checking that the ReachID is unique is one thing we could test for, and as we find more bugs, we can add them to that function, so that we don't have to try to catch them manually every single time.

That might deserve its own issue, but I only just thought of it. I'd appreciate your thoughts, @bangen.

wally-mac commented 6 years ago

@banderson1618, what is the status on this?

banderson1618 commented 6 years ago

@Albonicomt hasn't contacted me about his test results. The only thing holding me off from closing this is the fact that it hasn't been tested by a third party.

Albonicomt commented 6 years ago

@banderson1618 Here is the most recent run of the new BRAT tool I have run using the new segmenting tool https://usu.box.com/s/e49mqrqos7fjnfnd02usn5eeyfnm1i2x. I do remember checking the option to segment the network by roads, but it doesn't look like the function worked.The network is still segmented by the original 300m segements.

banderson1618 commented 6 years ago

I looked at the data. It seems to be working as expected. The tool doesn't remove the previously existing segments, it only splits apart segments that are intersected by a road. Because it seems to be working, I'm going to close this issue. If you think we should remove other segments, @Albonicomt or @wally-mac, let me know, and we can have that discussion.

Albonicomt commented 6 years ago

Oh nice! @banderson1618, my input is, since it is an optional function, we should keep it. It allows for somewhat of a customized analysis of a particular watershed. I don't think it hinders any part of the process or the output, it's definitely an enhancement. Always good to have options.

joewheaton commented 6 years ago

I'm going to open a new thread for referring back to this related to documentation.