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.

Sign up for GitHub

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

Jump to bottom

Constants precision nits #70

Open
MitchHeineman opened this issue Mar 19, 2021 · 6 comments · May be fixed by #190
Open

Constants precision nits #70

MitchHeineman opened this issue Mar 19, 2021 · 6 comments · May be fixed by #190
Assignees
@michaeltryby @cbuahin
Labels
addressed Issues that have been addressed. Pending merge before closing. enhancement
Milestone
v5.3.0

Comments

@MitchHeineman
Copy link

MitchHeineman commented Mar 19, 2021 •
edited
Loading

Very minor stuff: a few constants are applied inconsistently and/or could easily be resolved to higher precision. The gravity adjustment is less than 0.1%; the Manning's N coefficient adjustment is close to 0.3%

//consts.h
#define GRAVITY 32.2 // add precision to 32.174
#define SI_GRAVITY 9.81 // never used
#define PHI 1.486 // resolve to 1.4859 or exact representation as 0.3048^(-1/3)

//lid.c
LidProcs[j].surface.alpha =1.49 * sqrt(LidProcs[j].surface.surfSlope) / LidProcs[j].surface.roughness; // use PHI

  LidProcs[j].surface.alpha = 1.49 / LidProcs[j].surface.roughness * sqrt(LidProcs[j].surface.surfSlope);           // use PHI

  LidProcs[j].drainMat.alpha = 1.49 / LidProcs[j].drainMat.roughness * sqrt(LidProcs[j].surface.surfSlope);     // use PHI

//statsrpt.c
if (UnitSystem == US) Vcf = 7.48 / 1.0e6; // resolve to 7.4805

//subcatch.c
const double MCOEFF = 1.49; // use PHI

//transect.c
else Transect[j].hradTbl[i] = pow(qSum * Nchannel / 1.49 / aSum, 1.5); // use PHI

//link.c
aa = Conduit[k].beta / sqrt(32.2) * pow(Link[j].xsect.yFull, 0.1666667) * 0.3; // use GRAVITY
@dickinsonre
Copy link

Those 32.2 values have something to do with SWMM4 and SWMM3 - here is how it is defined in SWMM4

  GRVT                 = 32.2
  IF(METRIC.EQ.2) GRVT = 9.806
  IF(AMEN.EQ.0.0.AND.METRIC.EQ.1) AMEN = 12.566
  IF(AMEN.EQ.0.0.AND.METRIC.EQ.2) AMEN =  1.22

cbuahin added a commit that referenced this issue Aug 9, 2023
@cbuahin
Modified code to use consts instead of local values; bumped minor ver…
a831f4f 
…sion number #70
@cbuahin cbuahin self-assigned this Aug 9, 2023
@cbuahin cbuahin added this to the v5.2.5 milestone Aug 9, 2023
@cbuahin cbuahin mentioned this issue Aug 17, 2023
Merged
cbuahin added a commit that referenced this issue Aug 17, 2023
@cbuahin
Fixed code to use consts #70
9841cde
@michaeltryby
Copy link
Collaborator

@cbuahin SWMM users place a high value on consistency of results. This change is going to affect results for every model out in the wild. Seems like a lot for a patch release. I recommend holding off on this until the next minor update.

@dickinsonre
Copy link

Technical Detail:

The constant
PHI
PHI is defined as 1.486, although it could be resolved to 1.4859.
Significance:

The friction slope is one of the two most dominant terms in the dynamic wave solution of hydraulic models.
Potential Impact:

Even a slight alteration in the value of
PHI
PHI could have a significant impact on outfall flows. This is particularly relevant when considering that this term is used in every iteration, for every time step, and for all links in the model.
Recommendation:

Careful consideration should be given when defining the value of
PHI (testing)
PHI as even small changes could result in noticeable differences in model outputs.

@MitchHeineman
Copy link
Author

While I try to avoid excessive precision in reports, I like detail in my calcs. I suppose the ultimate change would be to change the code from frumpy US units to SI and conform with the rest of the world, whereby PHI would be relegated to the dustbin. But we seem to have bigger cultural fish to fry than metrification. Although NIST and NOAA are inching forward and retiring the US Survey foot (0.304801 m) in favor of the international foot (0.3048 m): https://oceanservice.noaa.gov/geodesy/international-foot.html

@cbuahin cbuahin modified the milestones: v5.2.5, v5.3.0 Aug 23, 2023
@cbuahin
Copy link
Collaborator

cbuahin commented Jan 19, 2024

Since we have decided to go with a minor update release, I think it is good to tackle this issue now.

cbuahin added a commit that referenced this issue Jan 24, 2024
@cbuahin
Modified code to use global consts instead of locally defined values #70
e3309b6
@cbuahin cbuahin added the addressed Issues that have been addressed. Pending merge before closing. label Jan 24, 2024
@cbuahin cbuahin closed this as completed Apr 15, 2024
@michaeltryby michaeltryby linked a pull request Oct 4, 2024 that will close this issue
Open
@michaeltryby
Copy link
Collaborator

Reopening this issue ...

Benchmarks are sensitive to numerical precision of constants

@michaeltryby michaeltryby reopened this Oct 8, 2024
@michaeltryby michaeltryby self-assigned this Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@michaeltryby michaeltryby

@cbuahin cbuahin

Labels
addressed Issues that have been addressed. Pending merge before closing. enhancement
Projects
None yet
Milestone
v5.3.0
Development

Successfully merging a pull request may close this issue.

Addresses Issue #70 -- Constants precision nits USEPA/Stormwater-Management-Model
4 participants
@dickinsonre @michaeltryby @cbuahin @MitchHeineman