r.fillnulls: avoid locale-dependent stderr parsing

[Abhi-d-gr8]

This PR fixes a locale-dependent bug in r.fillnulls when using bilinear/bicubic interpolation.

Instead of parsing English error messages from r.resamp.bspline stderr, the script now checks for NULL cells using r.univar -g and exits early when none are present.

This avoids failures in non-English locales and simplifies the control flow.

Fixes #1495.

[marisn]

Please add a test for it.
IIRC the reason of checking of stderr contents was that r.surf.bspline reported a non 0 exit status if no NULL cells were found. But non-0 exit status is considered to indicate a failure by code executing the command. Thus tests are needed to be sure that the code doesn't fail with a bizarre error when there is no error.

[Abhi-d-gr8]

Thanks for the clarification @marisn , and apologies for missing the test initially
I’m still new to contributing here.
I’ll add a regression test for the no-NULL case as suggested.

[Abhi-d-gr8]

Added a regression test (test_no_nulls) covering the no-NULL input case to ensure r.fillnulls exits successfully without relying on stderr parsing. CI is running.

[wenzeslaus]

I'm wondering if the right solution to the issue is modifying the underlying C tool rather than changing the Python code.

[wenzeslaus]

As @marisn mentioned in #1495, this might be a performance with a lot of data.

[marisn]

Indeed. As it requires a full scan (and probably resampling too!) of all computational region, it adds a nasty speed penalty. Then better to keep existing behavior as it works and is reasonably fast. I used r.fillnulls in a pipeline of over 11TB of data – every second counts.

[Abhi-d-gr8]

Hi ! @wenzeslaus
I went with the Python-side fix mainly to keep the change small and localized, and to avoid relying on parsing translated stderr output. The goal was to make sure r.fillnulls behaves correctly (and consistently across locales) when there are no NULL cells, and I added a regression test to cover that case.

I agree though that fixing this at the source in the underlying C module could be a cleaner long-term solution and benefit other callers as well. I also see your point about the extra r.univar -g call and the potential performance impact on large datasets.

I’m still getting familiar with the codebase, so I’m very open to guidance here. If you think adjusting the C tool is the better direction, I’m happy to explore that or rework this PR accordingly. Just let me know what you’d prefer.

[nilason]

I'm wondering if the right solution to the issue is modifying the underlying C tool rather than changing the Python code.

A possible solution might be to change the exit code here:

grass/raster/r.resamp.bspline/main.c

Line 459 in b6eba06

G_fatal_error(_("No NULL cells found in input raster."));

Exit code for G_fatal_error is by default EXIT_FAILURE (=1), which could be overridden by setting G_set_error_routine.

The new exit code can then be checked for in the test.

[nilason]

I'm wondering if the right solution to the issue is modifying the underlying C tool rather than changing the Python code.

A possible solution might be to change the exit code here:

grass/raster/r.resamp.bspline/main.c

Line 459 in b6eba06

G_fatal_error(_("No NULL cells found in input raster."));

Exit code for G_fatal_error is by default EXIT_FAILURE (=1), which could be overridden by setting G_set_error_routine.

The new exit code can then be checked for in the test.

Better still, set global errno.

[Abhi-d-gr8]

Hi ! @nilason and @wenzeslaus

Update: I reworked the fix based on the review feedback and pushed new commits.

• Moved the “no NULL cells” handling to the underlying C module (r.resamp.bspline): when -n is used and there are no NULL cells, it now exits successfully (instead of G_fatal_error() / EXIT_FAILURE).
• Removed the Python-side workaround in r.fillnulls (no more r.univar -g pre-scan), so there’s no extra pass over large rasters.
• Kept/updated the regression test (test_no_nulls) to ensure r.fillnulls succeeds for no-NULL inputs.

Could you please take another look when you get a chance? ... As well as thank you for the insights and happy to work if there are any follow ups or any other issues you think I can work upon

[nilason]

Moved the “no NULL cells” handling to the underlying C module (r.resamp.bspline): when -n is used and there are no NULL cells, it now exits successfully (instead of G_fatal_error() / EXIT_FAILURE).

With -n used, it is expected to produce an output raster. The only way to know if an output is produced, is with either EXIT_SUCCESS or EXIT_FAILURE. So G_fatal_error() must remain. What we can do is to set the global variable errno before the call to G_fatal_error with, let's say EINVAL. This error code may be checked in the test with python errno.

[marisn]

With -n used, it is expected to produce an output raster. The only way to know if an output is produced, is with either EXIT_SUCCESS or EXIT_FAILURE. So G_fatal_error() must remain. What we can do is to set the global variable errno before the call to G_fatal_error with, let's say EINVAL. This error code may be checked in the test with python errno.

Wrong topic. Discussion should be: "is it an error to call r.resamp.bspline -n with a map that has no NULL values?"
IMHO it is not, it should be just a warning. In a such case r.resamp.bspline should just copy input to output and be done. But we already once had this discussion and there were some arguments for it (can't find it now).

[nilason]

With -n used, it is expected to produce an output raster. The only way to know if an output is produced, is with either EXIT_SUCCESS or EXIT_FAILURE. So G_fatal_error() must remain. What we can do is to set the global variable errno before the call to G_fatal_error with, let's say EINVAL. This error code may be checked in the test with python errno.

Wrong topic. Discussion should be: "is it an error to call r.resamp.bspline -n with a map that has no NULL values?" IMHO it is not, it should be just a warning. In a such case r.resamp.bspline should just copy input to output and be done. But we already once had
this discussion and there were some arguments for it (can't find it now).

You might be right, but that’s for another PR.

[Abhi-d-gr8]

Thanks @nilason and @marisn, this discussion helped me understand the underlying concern much better.

I see your point now: the real question isn’t how to detect the no-NULL case, but whether calling r.resamp.bspline -n on a raster without NULLs should be treated as an error at all. From a user point of view it feels like a no-op, but I also get that this touches older design decisions and expected behaviour.

For this PR, I don’t want to reopen or change that broader behaviour unexpectedly. So I’m totally fine with keeping G_fatal_error() and not changing the success/failure contract of r.resamp.bspline.

If I’m understanding correctly, the preferred approach here would be:

1. keep the fatal error
2. set a specific errno (e.g. EINVAL) before calling it
3. update the test to check that instead of relying on translated stderr output
   That keeps the behaviour intact, avoids locale-dependent parsing, and keeps the fix scoped.

Or alternatively would you like me to go @wenzeslaus :

• revert the C-side change entirely and handle this only at the r.fillnulls level for now?

Please let me know if that sounds right ... I’ll adjust the PR accordingly and if the “no-NULL should just copy and warn” idea is better handled as a separate follow-up PR, I’m happy to leave it out of this one.

Thanks again for the guidance.

[nilason]

If I’m understanding correctly, the preferred approach here would be:

1. keep the fatal error
2. set a specific errno (e.g. EINVAL) before calling it
3. update the test to check that instead of relying on translated stderr output
   That keeps the behaviour intact, avoids locale-dependent parsing, and keeps the fix scoped.

Yes, that what I'd recommend for now.

Instead of:

grass/scripts/r.fillnulls/r.fillnulls.py

Line 593 in 9a994f4

if "No NULL cells found" in stderr:

check for errno.EINVAL.

(Current code actually does the copy in r.fillnulls that @marisn suggested should have been done already in r.resamp.bspline.)

[Abhi-d-gr8]

Hi! @nilason ,
Thanks for your guidance ... I’ve pushed an updated version addressing the points discussed above. Happy to adjust further if needed.

[nilason]

This can't work, you have to set errno in raster/r.resamp.bspline/main.c (and revert the changes there too).

[Abhi-d-gr8]

My bad ... I had some extra changes in main.c earlier which made this unclear. I’ve now reverted the extra bits and kept only the minimal errno = EINVAL before G_fatal_error().
I’ve updated that file accordingly and pushed the fix now.

[nilason]

My bad ... I missed that errno needs to be set in raster/r.resamp.bspline/main.c itself. I’ve updated that file accordingly and pushed the fix now.

No worries, but make sure to test locally before push to save precious CI (and maintainer) time.

[nilason]

Looks good to me now, thanks!
@petrasovaa Please take a final look at the python code.

[nilason]

The test is failing:

Running ./scripts/r.fillnulls/testsuite/test_r_fillnulls.py...
========================================================================
....EFAILED (errors=1)

======================================================================
ERROR: test_no_nulls (__main__.TestRFillNulls.test_no_nulls)
Test r.fillnulls when input raster has no NULL cells
----------------------------------------------------------------------
Traceback (most recent call last):
  File "scripts/r.fillnulls/testsuite/test_r_fillnulls.py", line 82, in test_no_nulls
    self.assertRasterFitsUnivar(
  File "etc/python/grass/gunittest/case.py", line 296, in assertRasterFitsUnivar
    self.assertModuleKeyValue(
  File "etc/python/grass/gunittest/case.py", line 243, in assertModuleKeyValue
    if not keyvalue_equals(
           ^^^^^^^^^^^^^^^^
  File "etc/python/grass/gunittest/checkers.py", line 393, in keyvalue_equals
    if not equal_fun(dict_a[key], dict_b[key], precision):
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "etc/python/grass/gunittest/checkers.py", line 298, in values_equal
    precision = float(precision)
                ^^^^^^^^^^^^^^^^
TypeError: float() argument must be a string or a real number, not 'NoneType'

----------------------------------------------------------------------
Ran 5 tests in 14.954s

FAILED (errors=1)
========================================================================
FAILED ./scripts/r.fillnulls/testsuite/test_r_fillnulls.py

[Abhi-d-gr8]

I see the test failure ... it’s coming from assertRasterFitsUnivar() being called without a precision in test_no_nulls, which leads to float(None) inside values_equal(). I’ll push a small fix adding an explicit precision for the null_cells check (doing precision=0) and rerun.
Or is it something else I am missing @nilason

[echoix]

I see the test failure ... it’s coming from assertRasterFitsUnivar() being called without a precision in test_no_nulls, which leads to float(None) inside values_equal(). I’ll push a small fix adding an explicit precision for the null_cells check (doing precision=0) and rerun. Or is it something else I am missing @nilason

@petrasovaa Even I tried to trace back where the precision gets defined/used, and couldn’t even answer myself if 0 made sense here.

from https://grass.osgeo.org/grass-devel/manuals/libpython/gunittest.html#gunittest.case.TestCase.assertRasterFitsUnivar, https://grass.osgeo.org/grass-devel/manuals/libpython/_modules/gunittest/case.html#TestCase.assertRasterFitsUnivar,
Passes down to https://grass.osgeo.org/grass-devel/manuals/libpython/_modules/gunittest/case.html#TestCase.assertModuleKeyValue

That is passed down to keyvalue_equals https://grass.osgeo.org/grass-devel/manuals/libpython/_modules/gunittest/checkers.html#keyvalue_equals that uses values_equal (probably in an unsafe way the way the default is in the signature if it could be mutable, I’m not sure for functions, but lists and dicts are immutable), https://grass.osgeo.org/grass-devel/manuals/libpython/_modules/gunittest/checkers.html#values_equal

In there, I’m not sure if the default value is still 0.000001 when it gets passed None explicitly. If that’s the case, the whole logic of that function doesn’t work anymore.
There’s no unit test of what happens when it receives None in the following place. Assuming we are getting it passed a value None by the wrappers.

grass/python/grass/gunittest/testsuite/test_checkers.py

Lines 30 to 70 in f258881

	class TestValuesEqual(TestCase):
	def test_floats(self):
	self.assertTrue(values_equal(5.0, 5.0))
	self.assertTrue(values_equal(5.1, 5.19, precision=0.1))
	self.assertTrue(values_equal(5.00005, 5.000059, precision=0.00001))
	self.assertFalse(values_equal(5.125, 5.280))
	self.assertFalse(values_equal(5.00005, 5.00006, precision=0.00001))
	self.assertFalse(values_equal(2.5, 15.5, precision=5))
	def test_ints(self):
	self.assertTrue(values_equal(5, 5, precision=0.01))
	self.assertFalse(values_equal(5, 6, precision=0.01))
	self.assertTrue(values_equal(5, 8, precision=3))
	self.assertFalse(values_equal(3600, 3623, precision=20))
	self.assertTrue(values_equal(5, 5))
	self.assertFalse(values_equal(5, 6))
	def test_floats_and_ints(self):
	self.assertTrue(values_equal(5.1, 5, precision=0.2))
	self.assertFalse(values_equal(5.1, 5, precision=0.01))
	def test_strings(self):
	self.assertTrue(values_equal("hello", "hello"))
	self.assertFalse(values_equal("Hello", "hello"))
	def test_lists(self):
	self.assertTrue(values_equal([1, 2, 3], [1, 2, 3]))
	self.assertTrue(values_equal([1.1, 2.0, 3.9], [1.1, 1.95, 4.0], precision=0.2))
	self.assertFalse(values_equal([1, 2, 3, 4, 5], [1, 22, 3, 4, 5], precision=1))
	def test_mixed_lists(self):
	self.assertTrue(values_equal([1, "abc", 8], [1, "abc", 8.2], precision=0.5))
	def test_recursive_lists(self):
	self.assertTrue(
	values_equal(
	[1, "abc", [5, 9.6, 9.0]], [1, "abc", [4.9, 9.2, 9.3]], precision=0.5
	)
	)

[Abhi-d-gr8]

Thanks @echoix for digging into this and confirming the root cause ... I agree about the explanation of the precision you are mentioning .

Though from my side, I don’t think there’s anything left to adjust in this PR now. Happy to adjust anything further if you or @nilason or @petrasovaa think something should be handled differently.

[echoix]

I don't think precision 0 is appropriate. It would mean a full unit of a number off would still pass. Is it that you had in mind?

[Abhi-d-gr8]

Thank you for the detailed investigation @echoix ! I understand your concern about precision=0, but I believe there might be a misunderstanding about what it means.

Looking at the values_equal function in checkers.py (lines 262-324), when precision=0:

For floats (lines 279-291), the code checks: if abs(value_a - value_b) > precision: Since both null_cells values are float(0), we get abs(0 - 0) > 0 which is False, so the values are considered equal.
For integers (lines 305-313), precision is only applied if int(precision) > 0. With precision=0, this condition is False, so it falls through to line 322 which does strict equality: elif value_a != value_b: return False

So precision=0 actually means "exact match required" with no tolerance, not "allow a difference of 1 unit".

Looking at the existing test suite (test_checkers.py), there are examples like:
Lines 290-295: precision=0 is used for exact integer comparisons
Line 42: values_equal(5, 8, precision=3) returns True (allows difference of 3)

In our case, we're checking that null_cells is exactly 0 (no tolerance), which is the correct behaviour for this test. Does this make sense, or am I missing something?

[nilason]

0.0 ?

[Abhi-d-gr8]

Hi @nilason !
Are you suggesting I should use precision=0.0 instead of precision=0 to make it clearer that it's a float comparison? Or are you referring to the null_cells: float(0) value itself? Happy to change either for clarity!
If it's the first case then I saw codebase using 0 everywhere and I think python does auto converts it accordingly...