improve floating point comparisons in vic_init.c #701

jhamman · 2017-03-15T16:45:43Z

A few cleanup items.

closes #xxx
tests passed
ran uncrustify prior to final commit
ReleaseNotes entry

yixinmao · 2017-03-15T20:47:24Z

vic/drivers/shared_image/src/vic_init.c

@@ -986,8 +986,7 @@ vic_init(void)
                }
                sum += soil_con[i].AreaFract[j];
            }
-            // TBD: Need better check for equal to 1.
-            if (sum != 1.) {
+            if (!assert_close_double(sum, 1.0, 0., 0.001)) {


It makes sense to not throw out a warning for very small sum errors. Is 0.001 enough a cutoff for not adjusting AreaFract though? (Similar for a couple of other changes below)

Let's have @tbohn and @bartnijssen weigh in here.

Options are:

Always divide by the sum and only print a warning when the difference is larger than a threshold (0.001).

Only divide by the sum when the difference is larger than a very small threshold (1e-20), print a warning in this case.

Raise an error in cases where the difference is greater than a very small threshold (1e-20) instead of doing work for the user.

Thoughts?

I guess option 3 allows for the simplest, easiest-to-maintain code. But it does place the burden on the user - is it possible to have a case where a user could have real trouble getting fractions to sum to within 1e-20 of 1.0 in their netcdf file? Or perhaps a better question is: would it be common enough for this to happen that VIC should be more accommodating?

Generally I prefer to have the work done by the user since that is the most explicit. They should only need to do this once and it seems to me that that would be an easy calculation in some version of nco, cdo or xarray. 0.001 would seem too large to me to take care of behind the scenes. The one thing is that our documentation needs to be very clear on what we expect from the user, so if we print an error message, then our documentation needs to be explicit on how to fix it or prevent it.

That said, I can live with 2.

Thanks @bartnijssen and @tbohn. I will update this PR to include a smaller threshold (probably 1e-10) and add clear warnings/documentation to that effect.

jhamman · 2017-04-28T13:53:32Z

@UW-Hydro/vic-developers - this is ready for another review.

bartnijssen

I'll wait with further review till it's resolved that this is what we want

bartnijssen · 2017-08-07T22:57:43Z

vic/drivers/shared_all/src/calc_root_fraction.c

@@ -42,6 +42,7 @@ calc_root_fractions(veg_con_struct  *veg_con,
    size_t               layer;
    size_t               zone;
    size_t               i;
+    size_t               max_itter;


should read max_iter. Actually it would make more sense if this were simply called iter or n_iter, since it is not the maximum number of iterations but just a counter

bartnijssen · 2017-08-07T23:00:01Z

vic/drivers/shared_all/src/calc_root_fraction.c

        while (zone < options.ROOT_ZONES) {
+            max_itter++;
+            if (max_itter > 9999) {


Replace 9999 by some #define or constant, e.g. MAX_ITER. Any reason why this is set to 9999 that seems a large number of iterations.

bartnijssen · 2017-08-07T23:00:29Z

vic/drivers/shared_all/src/calc_root_fraction.c

@@ -62,7 +63,16 @@ calc_root_fractions(veg_con_struct  *veg_con,
        Zsum = 0;
        zone = 0;

+        max_itter = -1;


Why not start at 0 and update the iterator at the end of the iteration rather than the beginning?

bartnijssen · 2017-08-07T23:01:31Z

vic/drivers/shared_all/src/calc_root_fraction.c

@@ -124,6 +134,7 @@ calc_root_fractions(veg_con_struct  *veg_con,
                }
            }
            else if (Zsum + Zstep > Lsum) {
+                zone++;   // should this be here?


No idea - shouldn't that be up to the person changing the code? At the very least the comment should be resolved (and removed) before the PR is accepted.

…t_library_shared_image.c

…tion sums

bartnijssen

Same comments as earlier. If you don't change them - just reply to the comment so I know you looked at it.

jhamman · 2017-08-08T16:03:00Z

vic/drivers/shared_all/src/calc_root_fraction.c

@@ -124,6 +134,7 @@ calc_root_fractions(veg_con_struct  *veg_con,
                }
            }
            else if (Zsum + Zstep > Lsum) {
+                zone++;


@tbohn - can you look at the changes in this file, specifically this line?

Looks good. I'm assuming you've tested this to ensure that the root fractions come out the same as before?

I wonder if there's a simpler algorithm for this function. Something to look into some day.

jhamman · 2017-08-08T17:57:00Z

@bartnijssen - I think we're all good here. Want to take one more look?

bartnijssen · 2017-08-08T18:31:12Z

@jhamman - review passed - just merging develop back into it since it said it was out-of-date. If the check passes I'll go ahead and merge.

yixinmao reviewed Mar 15, 2017

View reviewed changes

bartnijssen requested changes Aug 7, 2017

View reviewed changes

Joe Hamman and others added 11 commits August 7, 2017 16:34

Revert "Merge VIC5.0.1 release into develop branch"

076722c

improve floating point comparisons in vic_init.c and fix typo in prin…

070bd92

…t_library_shared_image.c

cleanup print library a bit

97d60de

possible fix to calc_root_fraction.c to avoid infinite loop

43c184a

support integer output types

4892027

uncrustify init fixes

8efdbec

replace missing Q12 in vic_run header

d8797e2

use minimum tolerance threshold defined in header file for area/eleva…

f69229a

…tion sums

divide Cv by Cv_sum

4a5ec66

roll back accidental changes

906cc84

add release notes entry

d9e5853

bartnijssen requested changes Aug 8, 2017

View reviewed changes

cleanup after review

56d1e84

jhamman commented Aug 8, 2017

View reviewed changes

bartnijssen approved these changes Aug 8, 2017

View reviewed changes

Merge branch 'develop' into feature/init_fixes

5e983fa

bartnijssen merged commit abdbe35 into UW-Hydro:develop Aug 8, 2017

jhamman mentioned this pull request Nov 1, 2017

Fixing the sum comparison #753

Open

This was referenced May 21, 2018

[Bug] Incorrect root fraction calculation #793

Closed

Fixed incorrect apportioning of root fractions. #794

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve floating point comparisons in vic_init.c #701

improve floating point comparisons in vic_init.c #701

jhamman commented Mar 15, 2017 •

edited

Loading

yixinmao Mar 15, 2017

jhamman Mar 16, 2017

tbohn Mar 16, 2017

bartnijssen Apr 4, 2017 •

edited

Loading

jhamman Apr 4, 2017

jhamman commented Apr 28, 2017

bartnijssen left a comment

bartnijssen Aug 7, 2017

bartnijssen Aug 7, 2017

bartnijssen Aug 7, 2017

bartnijssen Aug 7, 2017

bartnijssen left a comment

jhamman Aug 8, 2017

tbohn Aug 8, 2017

jhamman commented Aug 8, 2017

bartnijssen commented Aug 8, 2017

improve floating point comparisons in vic_init.c #701

improve floating point comparisons in vic_init.c #701

Conversation

jhamman commented Mar 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartnijssen Apr 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhamman commented Apr 28, 2017

bartnijssen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartnijssen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhamman commented Aug 8, 2017

bartnijssen commented Aug 8, 2017

jhamman commented Mar 15, 2017 •

edited

Loading

bartnijssen Apr 4, 2017 •

edited

Loading