search

smnorris / bcfishpass

Model and monitor aquatic habitat connectivity in BC. Tools to plan and prioritize the assessment and remediation of barriers.
https://smnorris.github.io/bcfishpass
Apache License 2.0
8 stars 13 forks source link

STUL_STUR_MIDR_TAKL_UTRE_LTRE_modelledfixtable_remove #585

Closed LauraB-CWF closed 4 weeks ago

LauraB-CWF commented 4 weeks ago

Remove fix entries from modelled crossing table for STUL_STUR_MIDR_TAKL_UTRE_LTRE watersheds.

smnorris commented 4 weeks ago

Thanks Laura. I'm not sure why GH isn't rendering the diff, but it looks like you've removed about 135 rows from this fix table. Were the fixes in these watersheds proving to be unreliable?

nickw-CWF commented 4 weeks ago

Thanks Laura. I'm not sure why GH isn't rendering the diff, but it looks like you've removed about 135 rows from this fix table. Were the fixes in these watersheds proving to be unreliable?

In short, yes. Quite a few of the fixes made by KK have comments like "to check - probable bridge", which I don't think we want treated as OBSs unless we are quite sure. I also reviewed some points that CWF staff did a few years ago, and we've refined our methods to be a bit more conservative when assigning OBS or no structure, so I think they warrant a full re-review as well.

For the partner maps we're creating, we're presenting an initial model run before any QA/QC or local knowledge integration so given the uncertainty around some of these fixes I wanted to start fresh and then we'll plan to do a full re-review of modelled crossings in these watersheds over the next couple weeks. Laura has copied the entries that she removed so that we have a record of them.

On our end though, there should have been 653 removed. However the structure field is blank for 397 of them, so I'm not sure what effect those were having on the model.

smnorris commented 4 weeks ago

It is tough for me to quickly tell exactly what is going on because the records have been re-ordered, resulting a substantial diff. Main branch file has 21879 records, this branch has 21744. If you're confident that all is good I can merge but for future adjustments preserving the row order will make things easier.

update - that row count might not be right

nickw-CWF commented 4 weeks ago

Everything should be good. For the future, how would we remove entries without re-ordering rows?

smnorris commented 4 weeks ago

Append new records to the bottom and remove existing records as needed (without sorting).

nickw-CWF commented 4 weeks ago

Ah ok, so it was the sorting that creates the issue, rather than removing entries. I'll make sure I share that with our technicians. Would it be easier to cancel this pull request and resubmit the updates with a non-sorted fix table?

smnorris commented 4 weeks ago

yes please!

smnorris commented 4 weeks ago

If sorting adds value, we could use some other method for comparing - git's diff isn't really designed for comparing data.

nickw-CWF commented 4 weeks ago

I think this is fine for now, but it is a little bit cumbersome because the rows have to be deleted individually which takes some time with >600 rows. I don't necessarily see wholescale deletions in the future, I think this is an isolated case.