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

Precision loss when calculating the TotalDuration, as a result, the last report result may not be added in output file #219

Merged
merged 2 commits into from
Feb 25, 2019

Conversation

WaterDesk
Copy link

@WaterDesk WaterDesk commented Jan 14, 2019

Precision loss when calculating the TotalDuration, as a result, the last report result may not be added in output file

SWMM version: 5.1.013

Reproduce Steps:

  1. Use the sample INP file “Example1.inp”, make some changes:
    • END_TIME changes to 11:30 from 12:00
    • REPORT_STEP changes to 00:30:00 from 01:00:00
  2. Open the changed "Example1.inp" file, then Run Simulation
  3. Pick up a subcatchment or node or link, and Create a Time Series Table, then you will find the result data of the END_TIME has not been listed in the table. The current last one is Days 1 Hours 11:00:00, but not 11:30:00.

Analysis:

The Start/End Date and Time is stored as text in INP file, after parsing them from INP file, they will be converted to double values.

The EndTime “11:30:00’’ converts to 0.47916666666666669, when it is added with EndDate “01/02/1998” (35797.000000000000), the new EndDateTime is 35797.479166666664.
Use the below method to calculate the TotalDuration, the result is 127799.0.

    TotalDuration        = floor((EndDateTime - StartDateTime) * SECperDAY);

From the INP file, the StartDateTime is 01/01/1998 00:00:00, and the EndDateTime is 01/02/1998 11:30:00. The duration is 1 day and 11:30:00, it has 127800 seconds.

1 second precision losses now! So the last time result will never been reported in output file.

Solution:

The precision loss usually happens when a large double value plus a small double value. So, we need to avoid such calculations.

Change the calculation method of TotalDuration to be:

        double durationDate = EndDate - StartDate;
        double durationTime = EndTime - StartTime;
        TotalDuration = floor(durationDate * SECperDAY + durationTime * SECperDAY);

…ast report result may be added in output file
@WaterDesk WaterDesk changed the title Precision loss when calculating the TotalDuration, as a result, the last report result may be added in output file Precision loss when calculating the TotalDuration, as a result, the last report result may not be added in output file Jan 14, 2019
@dickinsonre
Copy link

Hi, If you are using the Delphi SWMM5 GUI - does this happen if you save the date changes to the inp file before running the model?

@WaterDesk
Copy link
Author

@dickinsonre I was using official SWMM installed from https://www.epa.gov/sites/production/files/2018-08/swmm51013_setup_1.exe.

Yes, I saved the date changes to the inp file and run the model. I found the issue from the UI operations, So I debugged into the code, and then I verified my thought.

@dickinsonre
Copy link

Thanks for the clarification, @WaterDesk and thanks for finding a fix.

@bemcdonnell
Copy link
Member

@WaterDesk, Thanks for reporting this issue. @LRossman / @dickinsonre , do you have an opinion on this?

@dickinsonre
Copy link

I am having trouble duplicating this problem. I change the date,/time run the model and see the new ending date. I am still checking.

@LRossman
Copy link

I can reproduce the problem. I believe this fix will also work:

   TotalDuration = floor( ( (EndDateTime - StartDateTime) * SECperDAY) + 0.5);

@dickinsonre
Copy link

I can reproduce the problem. I believe this fix will also work:

   TotalDuration = floor( ( (EndDateTime - StartDateTime) * SECperDAY) + 0.5);

Sounds good Lew, I still cannot duplicate this in other models. I seem to be able to change the date at will.

@LRossman
Copy link

@WaterDesk 's fix is better than mine because the numbers being subtracted from one another are closer in value.

@bemcdonnell
Copy link
Member

@WaterDesk (and @michaeltryby) this change is going to require us to update the benchmarks for the regression tests. @michaeltryby, Is there perhaps a side discussion we can have to improve the workflow for updating the benchmarks?

@WaterDesk
Copy link
Author

Aha, it would be better to have a simpler process to run and update the benchmarks.

@michaeltryby
Copy link

@bemcdonnell opened up Issue #221 for side discussion on reg testing workflow.

@michaeltryby
Copy link

@WaterDesk I have made some changes in develop to simplify running reg tests and updating the benchmark. I am going to merge these changes into your branch, update the benchmark and then proceed to merge your PR. Thanks for your patience!

@michaeltryby
Copy link

@WaterDesk I don't have permission to merge upstream develop into your branch. You can grant me permission or do it yourself, either way is fine with me. Let me know what you decide. Thanks!

@michaeltryby michaeltryby merged commit 4a14d5c into pyswmm:develop Feb 25, 2019
@michaeltryby
Copy link

@WaterDesk disregard my last message I found a workaround. Thanks for your bug fix!

@WaterDesk
Copy link
Author

@michaeltryby Thanks for you merge!

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.

5 participants