Skip to content

temporal: Unused variables in temporal modules #1082

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

Closed
nobeeakon wants to merge 1 commit into from
Closed

temporal: Unused variables in temporal modules #1082

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion temporal/.flake8
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
[flake8]
ignore =
F841, # local variable 'column' is assigned to but never used
F821, # undefined name '_'
E265, # block comment should start with '# '
E266, # too many leading '#' for block comment
Expand Down
4 changes: 0 additions & 4 deletions temporal/t.rast.algebra/testsuite/test_raster_algebra.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,7 +392,6 @@ def test_tmap_function1(self):

D = tgis.open_old_stds("R", type="strds")

maplist = D.get_registered_maps_as_objects()
Copy link
Copy Markdown
Member

@wenzeslaus wenzeslaus Nov 16, 2020

Choose a reason for hiding this comment

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

Similarly to the ret = grass.script.run_command line, these also do something, but since the test is about something else there seems to be no reason to just test if get_registered_maps_as_objects() executes, nor further investigate its result. So, I agree with removing those.

self.assertEqual(D.metadata.get_number_of_maps(), 1)
self.assertEqual(D.metadata.get_min_min(), 99)
self.assertEqual(D.metadata.get_max_max(), 99)
Expand All @@ -409,7 +408,6 @@ def test_tmap_function2(self):

D = tgis.open_old_stds("R", type="strds")

maplist = D.get_registered_maps_as_objects()
self.assertEqual(D.metadata.get_number_of_maps(), 1)
self.assertEqual(D.metadata.get_min_min(), 100)
self.assertEqual(D.metadata.get_max_max(), 100)
Expand All @@ -426,7 +424,6 @@ def test_map_function1(self):

D = tgis.open_old_stds("R", type="strds")

maplist = D.get_registered_maps_as_objects()
self.assertEqual(D.metadata.get_number_of_maps(), 4)
self.assertEqual(D.metadata.get_min_min(), 101)
self.assertEqual(D.metadata.get_max_max(), 104)
Expand All @@ -443,7 +440,6 @@ def test_map_function2(self):

D = tgis.open_old_stds("R", type="strds")

maplist = D.get_registered_maps_as_objects()
self.assertEqual(D.metadata.get_number_of_maps(), 4)
self.assertEqual(D.metadata.get_min_min(), 100)
self.assertEqual(D.metadata.get_max_max(), 400)
Expand Down
1 change: 1 addition & 0 deletions temporal/t.rast.to.vect/t.rast.to.vect.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,7 @@ def main(options, flags):
output="dummy", run_=False,
finish_=False, flags=flags,
type=method, overwrite=overwrite,
column=column,
Copy link
Copy Markdown
Member

@wenzeslaus wenzeslaus Nov 16, 2020

Choose a reason for hiding this comment

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

Good catch. Do you also consider the previous state a bug?

Mentioning @veroandreo to have this confirmed.

Copy link
Copy Markdown
Contributor

@veroandreo veroandreo Nov 16, 2020

Choose a reason for hiding this comment

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

Well, I'm not sure. This is a call to r.to.vect which default for column parameter is value. Hence dunno if needed here. In the end the module converts each raster from a strds into a vector and registers them as stvds, what other thing would we store when converting a raster to a vector if not the value? All the examples in r.to.vect omit the column parameter

Copy link
Copy Markdown
Member

@wenzeslaus wenzeslaus Nov 17, 2020

Choose a reason for hiding this comment

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

In both cases, the documentation says it is Name of attribute column to store value. So according to the doc, it is a name, not what should be be stored. (In theory, you could store, e.g., color, but that's not the case here.) Then it makes sense to pass it and since there is a default which is often good enough, it is not surprising that nobody complained.

Copy link
Copy Markdown
Contributor

@veroandreo veroandreo Nov 17, 2020

Choose a reason for hiding this comment

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

Yes, it puts value as default column name. Since there's no option to store any other attribute, I believe it's a reasonable default name. Of course, maybe better to have it explicitly there in case eventually r.to.vect allows to store something else and hence the name value would no longer apply

quiet=True)

# The module queue for parallel execution, except if attribute tables should
Expand Down
1 change: 0 additions & 1 deletion temporal/t.rast.what/t.rast.what.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -383,7 +383,6 @@ def one_point_per_col_output(separator, output_files, output_time_list,
for count in range(len(output_files)):
file_name = output_files[count]
gscript.verbose(_("Transforming r.what output file %s"%(file_name)))
map_list = output_time_list[count]
in_file = open(file_name, "r")
lines = in_file.readlines()

Expand Down
3 changes: 1 addition & 2 deletions temporal/t.rast3d.algebra/testsuite/test_raster3d_algebra.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,7 @@ def setUpClass(cls):
os.putenv("GRASS_OVERWRITE", "1")
tgis.init(True) # Raise on error instead of exit(1)
cls.use_temp_region()
ret = grass.script.run_command("g.region", n=80.0, s=0.0, e=120.0,
w=0.0, t=100.0, b=0.0, res=10.0)

Copy link
Copy Markdown
Member

@wenzeslaus wenzeslaus Nov 16, 2020

Choose a reason for hiding this comment

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

Did you try to run this test? This call has an "side effect" meaning it does something else than just generate the return value, specifically it sets the computational region to whatever the test is meant to be executed with. The run_command() return value can be ignored in this case (it should be None actually). Removing the variable is the solution, while the call needs to stay in place.

Copy link
Copy Markdown
Contributor Author

@nobeeakon nobeeakon Nov 17, 2020

Choose a reason for hiding this comment

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

Great, thanks for the comments, yeah, completely missed the testing. However, I'm not completely sure how to run them locally, if you could give me some light on that I would really appreciate it.

Copy link
Copy Markdown
Member

@wenzeslaus wenzeslaus Nov 18, 2020

Choose a reason for hiding this comment

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

For running tests you need NC SPM sample dataset. Use the alpha version from http://fatra.cnr.ncsu.edu/data/nc_spm_full_v2alpha2.tar.gz to have the same one used in the CI.

The commands to run the tests are at the following page under Running tests and creating report and under Running individual test files. You can run a single file or a (sub)directory.

https://grass.osgeo.org/grass79/manuals/libpython/gunittest_running_tests.html


cls.runModule("r3.mapcalc", overwrite=True, quiet=True, expression="a1 = 1")
cls.runModule("r3.mapcalc", overwrite=True, quiet=True, expression="a2 = 2")
Expand Down
1 change: 0 additions & 1 deletion temporal/t.vect.algebra/t.vect.algebra.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@ def main():
expression = options['expression']
basename = options['basename']
spatial = flags["s"]
stdstype = "stvds"

# Check for PLY istallation
try:
Expand Down