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

r.buffer: Added test script #4482

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Shreshth-Malik
Copy link

This PR adds regression tests for the r.buffer module to ensure its functionality remains intact with future code changes. The following test cases are covered:

test_buffer_creation(): Verifies the creation of buffer zones around raster features based on specified distances. It checks that the output raster exists, has expected category values starting from 1, and that the minimum and maximum values match the expected range.

test_no_non_null_values(): Confirms that the r.buffer module correctly handles an input raster that is entirely null, ensuring the output contains no non-null values.

test_ignore_zero_values(): Ensures that zero values in the input raster are ignored when creating buffer zones, resulting in an output that does not include any zero categories.

@github-actions github-actions bot added raster Related to raster data processing Python Related code is in Python module tests Related to Test Suite labels Oct 9, 2024

self.assertRasterExists(output)

expected_categories = [1] + [i + 1 for i in range(len(distances) + 1)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you want 1 twice?

Suggested change
expected_categories = [1] + [i + 1 for i in range(len(distances) + 1)]
expected_categories = [i + 1 for i in range(len(distances) + 1)]

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the first 1 is the default category of the road, then i start i from i+1 because i already have a default 1 in the expected category list. The last +1 in len(distances) + 1 is because categories start from index 1.

I will conduct further tests to understand this in a deeper context, and apply corrections accordingly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also look at the start and stop parameters of range if you'd feel it would make it simpler

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the suggested change

Comment on lines 54 to 68
category_values = gs.read_command(
"r.stats", flags="n", input=output
).splitlines()
category_values = [int(line.split()[0]) for line in category_values]

print("Category values:", category_values)

unique_actual_categories = set(category_values)
self.assertTrue(
unique_actual_categories.issubset(set(expected_categories)),
msg=(
"Output categories should be a subset of expected categories: "
+ str(expected_categories)
),
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to test the exact categories, than the previous min/max test is somewhat redundant. Why subset, the categories should be compared exactly, no?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thought process here was to first check if the categories are in the expected min max range. However, it doesn't verify if all expected categories are present or if there are extra categories.

Second check was to make sure there are no extra categories or abnormal categories that the user didn't want.

But I can see how it can be redundant. I will work on patching this and making sure a single test is sufficient.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the redundant test


self.assertRasterExists(output)
stats = gs.read_command("r.univar", map=output, flags="g")
self.assertIn(
Copy link
Contributor

@petrasovaa petrasovaa Oct 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this test I use r.univar to ensure that the output map has no non-null cells when the input is entirely null. According to my understanding r.univar helps calculate univar stats and if it is a null output it gives out n=0 (number of non null cells). This verifies that the buffer operation correctly handles null input data without introducing any non-null values.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used assertRasterFitsUnivar instead of manually doing it

print("Category values:", category_values)

self.assertNotIn(
0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r.buffer won't output 0 in any case, so it looks like it does not test what you want it to test.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will work on patching it and diving deeper to understand the exact expected outputs in a few edge cases.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compared with expected_categories = [1,2] instead of 0

refmin=min(expected_categories),
refmax=max(expected_categories),
msg=(
"Buffer zones should have category values from 1 to " # Checking if there are no abnormal values in the output raster map
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use f string here


expected_categories = [1, 2]

self.assertNotIn(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is incorrect, it will pass even if category_values are empty. Could you better explain what exactly are you trying to test here? Also distances can be just 100, since given the input, it will never get to 200 meters...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I apologize for the multiple changes, I will be more careful with them. New to the team and still learning.
Will recheck these and push the changes soon.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main idea here was to make a map with alternating rows of zero and one values. The first check is to see if it indeed made an output map and second is to verify there are no 0 values in the output as -z flag will help ignore zero values in output when creating buffers. This should verify that the -z flag works properly and confirms that buffer zones are not created around zero values and they are ignored.

I made a mistake in the assert. It should have been:
self.assertNotIn( 0, category_values, msg="Output should not contain buffer zones around zero values", )

Do you recommend keeping the test after making the above change and removing expected_categories?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There will never be 0 in the output, the output values always start with category 1 ("distances calculated from these locations"). So this test is not very useful as it is now. Perhaps you can create a raster with zeros and then run it with -z and then you should get an output raster with nulls only, because the zeroes are ignored and there are no source cells for the buffer, hence nulls everywhere. I recommend running it with different scenarios to make sure you understand what it's doing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module Python Related code is in Python raster Related to raster data processing tests Related to Test Suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants