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

tests: Call t.remove and g.remove once with multiple inputs as list #4523

Merged
merged 8 commits into from
Oct 18, 2024

Conversation

echoix
Copy link
Member

@echoix echoix commented Oct 14, 2024

This PR should help squeeze out a couple of seconds of testing time by calling t.remove or g.remove once with multiple inputs separated by a comma, when all other parameters are the same. For t.remove, I combined the layers in the inputs argument with a comma, and for g.remove, I combined the names of the files of the name argument. I referred to https://grass.osgeo.org/grass85/manuals/t.remove.html and https://grass.osgeo.org/grass85/manuals/g.remove.html.

I tried to keep using a manual string concatenation to be as close as the usage shown in the docs. However, when changing g.remove usages, I saw that, at least in gunittests self.runModule, name could be passed a list or a tuple for a similar effect. I tried to follow the call chain for runModule, but couldn't pinpoint with precision where that conversion happened. So, I kept the changes safe and close to the existing tests. If you can show me how safe it is to pass a list (or tuple), some changes could be made simpler.

Last week I benchmarked a single test in t.rast.accumulate and these changes had a measurable impact, being 1.18x faster (when combined with removing unused SimpleModule import, like filed in #4521).

I still kept the test prefix in the title, even if some changes are for benchmarks, and one change in utils/thumbnails.py

@github-actions github-actions bot added vector Related to vector data processing raster Related to raster data processing temporal Related to temporal data processing Python Related code is in Python libraries module imagery tests Related to Test Suite raster3d labels Oct 14, 2024
@petrasovaa
Copy link
Contributor

I tried to keep using a manual string concatenation to be as close as the usage shown in the docs. However, when changing g.remove usages, I saw that, at least in gunittests self.runModule, name could be passed a list or a tuple for a similar effect. I tried to follow the call chain for runModule, but couldn't pinpoint with precision where that conversion happened. So, I kept the changes safe and close to the existing tests. If you can show me how safe it is to pass a list (or tuple), some changes could be made simpler.

I am pretty sure you can use lists here.

@wenzeslaus
Copy link
Member

+1 for the lists. If self/cls.runModule does not work, you can switch to gs.run_command. Main reason for runModule was to track execution and align with assertModule and neither of those is particularly strong reason to keep using runModule now. Otherwise, the string concatenation would be done with ",".join([item1, item2, item3]).

@echoix
Copy link
Member Author

echoix commented Oct 15, 2024

+1 for the lists. If self/cls.runModule does not work, you can switch to gs.run_command. Main reason for runModule was to track execution and align with assertModule and neither of those is particularly strong reason to keep using runModule now. Otherwise, the string concatenation would be done with ",".join([item1, item2, item3]).

If it doesn't work, there's a bigger problem, as it is used at multiple places.

I'll try to do the changes next time I have time to code :)

@echoix echoix merged commit 6fd21a8 into OSGeo:main Oct 18, 2024
27 checks passed
@echoix echoix deleted the remove-multiple-layers branch October 18, 2024 13:39
@github-actions github-actions bot added this to the 8.5.0 milestone Oct 18, 2024
@a0x8o a0x8o mentioned this pull request Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
imagery libraries module Python Related code is in Python raster Related to raster data processing raster3d temporal Related to temporal data processing tests Related to Test Suite vector Related to vector data processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants