Skip to content
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

STUL_STUR_MIDR_TAKL_UTRE_LTRE_modelledfixtable_remove #585

Closed

Conversation

LauraB-CWF
Copy link
Collaborator

Remove fix entries from modelled crossing table for STUL_STUR_MIDR_TAKL_UTRE_LTRE watersheds.

Remove fix entries from modelled crossing table for STUL_STUR_MIDR_TAKL_UTRE_LTRE watersheds.
@smnorris
Copy link
Owner

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
Copy link
Contributor

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
Copy link
Owner

smnorris commented Oct 25, 2024

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
Copy link
Contributor

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

@smnorris
Copy link
Owner

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

@nickw-CWF
Copy link
Contributor

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
Copy link
Owner

yes please!

@LauraB-CWF LauraB-CWF closed this Oct 25, 2024
@smnorris
Copy link
Owner

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

@nickw-CWF
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants