Kerry's tests: SL=0 issue?

r-barnes / Barnes2019-DepressionHierarchy

Fast handling of depressions in flow graphs

MIT License

9 stars 7 forks source link

Kerry's tests: SL=0 issue? #9

Open awickert opened 4 years ago

awickert commented 4 years ago

~~It seems that Kerry's ASCII tests are failing when sea level = 0.~~
run_tests.sh will not work because of the altered output name and different structure.

What is the cause of the former, and is it worth updating the latter?

r-barnes commented 4 years ago

We should probably try to update the tests. If they still check out, then it's good protection against regressions. I'm sorry I didn't catch this earlier.

awickert commented 4 years ago

Update: with your version of the code (instead of my fork), the ASCII tests succeed at SL=0.

After revision, perhaps we should set them up with Travis.

No apologies necessary!

KCallaghan commented 4 years ago

I think those would not have worked with SL = 0 because the SL value in those files was not 0, it was a negative number. I think we should throw an error if the user attempts to use a sea level that does not exist in the input file! This is one of the changes that I have made in my open pull request that will warn a user if their 'sea level' selection is invalid.

awickert commented 4 years ago

Thanks @KCallaghan. I had misunderstood the use of "sea level" after some tests with your examples that... somehow worked. Suggested action items:

Change the term to "ocean value" or "ocean flag" to match the use of "ocean" in the paper and to indicate to users that it is a single value
[agree with @KCallaghan] Throw an error if value does not exist on the DEM

awickert commented 3 years ago

bump

How about updating the documentation to note that "sea level" is actually not a level -- meaning that all values < SL will not be flooded. Rather, it is a tag to indicate the ultimate sink.