kcigeospatial / MDOT-SHA-NPDES-Next-Gen

Code and issues related to the MDOT SHA NPDES Project. Project codes: Config = 31, Management = 32.
0 stars 0 forks source link

BMP Inspection ETL Issues - Upstream/Downstream Embankment and Riser Structure #211

Closed KCI-Ablowers closed 6 years ago

KCI-Ablowers commented 6 years ago

@johnshiu

johnshiu commented 6 years ago

@KCI-Ablowers

For the EMBD/EMBU issue, this is what I have in the system check; I'm not sure if I'm understanding it properly. This is checking for non-zero embankment fields, and then if any exist, it checks whether dam height is zero or unpopulated. If that occurs, an error is thrown. Is this incorrect?

image

I guess, is the error checking for these two issues incorrect, or is it just the error message that needs to be updated?

brentreeves75 commented 6 years ago

if DAM_HEIGHT is zero or blank then EMBU and EMBD inspection values should be Not Part of Design. And reverse: if any of the EMBU or EMBD inspection values are anything other than Not Part of Design, then DAM_HEIGHT is not zero or blank.

johnshiu commented 6 years ago

I have added the reverse case for the EMBD/EMBU/DAM_HEIGHT issue.

For the second one, the existing logic looks fine to me: basically, if any of the rsr_open/sedi/strc is non-zero, then all of them must be nonzero. Is the second issue being flagged incorrectly? If so, is there an example case?

KCI-Ablowers commented 6 years ago

For the second one: In the survey, the Riser Structure being set to Not Part of Design will leave the other fields (Sediment, Opening, Trash Rack, and Valve) hidden. So this flag is correct, but it is not possible to fill in the attributes for Sediment and Opening based on the survey forms configuration.

johnshiu commented 6 years ago

Ah, I see the issue with that.

Should the field ETL make updates to the survey (i.e. auto-zero things out) if RSR_STRC is set to Not Part of Design? I wonder if this would be dangerous if it was accidental?

Since this is S123 related, looping in @talllguy

KCI-Ablowers commented 6 years ago

I'll leave that one up to @talllguy, but maybe it would be better to just write it the other way. If Riser Structure has anything other then NPD (or maybe NR) then Open and Sediment should be populated with anything not equal to NPD.

johnshiu commented 6 years ago

Okay, I can update the ETL error message for now.

talllguy commented 6 years ago

In Survey123, if a field is hidden by relevance, the value us written as NULL, just as Andrew points out in https://github.com/kcigeospatial/MDOT-SHA-NPDES-Next-Gen/issues/211#issuecomment-390733747

Since we cannot write to those hidden fields, I think John's approach (where the ETL sets those to 0) would be the best solution. There shouldn't be an issue with that affecting form editing because the Survey form should ignore those 0 values.

johnshiu commented 6 years ago

FYI, this is not yet implemented.

johnshiu commented 6 years ago

The in-place field ETL now will auto-set all RSR_* fields to 0 if RSR_STRC was set to 0.

johnshiu commented 6 years ago

This is ready for use at SHA.

talllguy commented 6 years ago

@KCI-Ablowers Test this one out, document and close.

KCI-Ablowers commented 6 years ago

The Riser issue has been fixed in SHA. The null/empty field show in the inspections export CSV- image

@johnshiu and @talllguy Since there has been some discussion in the last two meetings with firms about the Embankment and Dam Height issue, I think we need to tweak the rule. We either need to make those embankment fields empty like it is set up for riser OR we need to change the code to look for Not Part of Design. I know '0' in the survey is NPD, but for some reason, this flag appears in almost every survey we have seen in the past two days. I think fixing it instead of saying its an "exemption" makes more sense moving forward.

talllguy commented 6 years ago

Here is a matrix for what I propose to be flagged, if not already. @johnshiu Can you translate this into code and paste it in here to verify?

ETL Flag Test # SWMFAC.DAM_HEIGHT BMP_BMPINSPECTIONS.EMB* (any EMB)
No flag ✅ 106-112 0 0
No flag ✅ 106-112 NULL 0
Flag 🔻 223 10 0
No flag ✅ 223 10 3
Flag 🔻 106-112 0 3
Flag 🔻 106-112 NULL 3
johnshiu commented 6 years ago

The rule as it is written now works like this:

if (dam_height == (0 or null)) and (any emb != (0 or null)): error is flagged if (dam_height != (0 or null)) and (any emb == (0 or null)): error is flagged

I think this is the desired behavior, right? And it should pass those test cases that Elliott wrote.

Are there instances in the BMP inspections where this is not supposed to be flagged but is? If so, can you provide the dam_height/emb field values?

talllguy commented 6 years ago

John, that looks right to me. @KCI-Ablowers can you provide any examples of where the flag is false positive? I recall it seeming to be an issue when we were showing the flags to RJM.

KCI-Ablowers commented 6 years ago

SWMFAC# 020174, 020175, 020176 all have a dam height = 0 in the inventory feature service. All three have zeros (Not Part of Design) in the inspection summary CSV (Screen cap below)

image

johnshiu commented 6 years ago

@KCI-Ablowers, thanks for the example; I'll check it out.

johnshiu commented 6 years ago

@KCI-Ablowers, okay, I believe I have fixed the issue. It was checking for the number 0 instead of the string '0', so it was not picking things up properly. I've added '0' and 'nr' as possible matches, and it should detect things properly now. Please let me know if this still causes issues.

KCI-Ablowers commented 6 years ago

@johnshiu Sounds good. I will look for this in our next push or ask RJM to run the Validate process and report back if the error is cleared. Or, I can enter a test inspection to see. What would be the best test case?

johnshiu commented 6 years ago

@KCI-Ablowers I think a full RJM re-run would be best since they have so many. I think we should expect the vast majority to be cleared.

KCI-Ablowers commented 6 years ago

@johnshiu I will shoot them an email and ask if I can run the validate tool for them so we can test.

KCI-Ablowers commented 6 years ago

@johnshiu This is fixed. RJM re-synced and validated their data for me this morning. There are 4 instances of the error now. The script is correctly identifying zero or null dam heights in SWMFAC and embankment ratings in the survey. Think we can close this one!