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

Appveyor CI: Fixing Regression Tests #161

Closed
bemcdonnell opened this issue Mar 9, 2018 · 3 comments
Closed

Appveyor CI: Fixing Regression Tests #161

bemcdonnell opened this issue Mar 9, 2018 · 3 comments

Comments

@bemcdonnell
Copy link
Member

Lateral Inflow Bug:

A few months ago we discovered what we thought was a redundant line in the lateral inflow (see #61). Turns out that this change resulted in us failing two regression tests. Lateral inflows were off by 0.001. This has been reverted back in the next PR coming in today.

System Inflow Totalizers:

This change will actually be somewhat disruptive. Before, when SWMM had a negative inflow through an outfall, it was not counted as a positive inflow to the system. This change is the following:

--- a/src/routing.c
+++ b/src/routing.c
@@ -804,7 +804,7 @@ void removeOutflows(double tStep)
         // --- update mass balance with flow and mass leaving the system
         //     through outfalls and flooded interior nodes
         q = node_getSystemOutflow(i, &isFlooded);
+        if ( q > 0.0 ) // <-----------------------------------------------
-        if ( q != 0.0 )
         {
             massbal_addOutflowFlow(q, isFlooded);
             for ( p = 0; p < Nobjects[POLLUT]; p++ )
@@ -813,7 +813,7 @@ void removeOutflows(double tStep)
                 massbal_addOutflowQual(p, w, isFlooded);
             }
         }
+        else massbal_addInflowFlow(EXTERNAL_INFLOW, -q);// <-------------------

         // --- update mass balance with mass leaving system through negative
         //     lateral inflows (lateral flow was previously accounted for)

This change requires us update the benchmarks for user3

@bemcdonnell bemcdonnell added this to the v5.2.0.dev3 milestone Mar 9, 2018
@bemcdonnell bemcdonnell self-assigned this Mar 9, 2018
@dickinsonre
Copy link

Good catch @bemcdonnell

@bemcdonnell bemcdonnell added the type:update-benchmark Task Requires an update to the Benchmark Regression Tests label Mar 9, 2018
@michaeltryby
Copy link

Looks like test "User 3" fails due to this change in the flow totalizer. So I guess we need to update the benchmark. We need to develop a process for doing this so we can keep track of how the benchmark changes over time and be able to reproduce benchmark results. Any thoughts?

@dickinsonre
Copy link

How about just a note in the note section of the input file showing the old flow equalizer total and explaining the change in the code. Or, how about a a more multi objective test that compares all of the flow totals in the links of the two model output files.

@bemcdonnell bemcdonnell removed the type:update-benchmark Task Requires an update to the Benchmark Regression Tests label Aug 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants