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.

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

v.surf.icw: Python3 and modern GRASS compatibility, fix bug introduced in trac # 2574 #1200

Open
wants to merge 13 commits into
base: grass8
Choose a base branch
from
Open

v.surf.icw: Python3 and modern GRASS compatibility, fix bug introduced in trac # 2574 #1200

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
3cddff2
v.surf.icw: modernize for Python3 and current GRASS parser behavior
HamishB Sep 5, 2024
09c0c2c
v.surf.icw: bugfix incomplete change in trac bug # 2574
HamishB Sep 5, 2024
392143b
v.surf.icw: deal to trailing whitespace
HamishB Sep 5, 2024
f9f4b5e
Merge branch 'grass8' into vsi_python2_to_3
echoix Sep 5, 2024
34f475e
v.surf.icw: modernize for Python3 and current GRASS parser behavior
HamishB Sep 5, 2024
f4b5d74
v.surf.icw: bugfix incomplete change in trac bug # 2574
HamishB Sep 5, 2024
dc38a04
v.surf.icw: deal to trailing whitespace
HamishB Sep 5, 2024
3fd8e45
v.surf.icw: make indent checks happy
HamishB Sep 5, 2024
9f6b626
Merge branch 'vsi_python2_to_3' of github.com:HamishB/grass-addons in…
HamishB Sep 6, 2024
5a407e7
v.surf.icw: make less chatty
HamishB Sep 6, 2024
34e26cc
v.surf.icw: make less spacey
HamishB Sep 6, 2024
8eba0ef
Use G_OPT_ standard parser options. This changes workers= to nprocs=.
HamishB Sep 12, 2024
12a709c
v.surf.icw: add screenshot to help page
HamishB Sep 12, 2024
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
3 changes: 2 additions & 1 deletion src/vector/v.surf.icw/v.surf.icw.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,5 +134,6 @@ <h2>SEE ALSO</h2>
<h2>AUTHOR</h2>

Hamish Bowman<br>
<i>Department of Marine Science,<br>
<i>Department of Geology<br>
University of Otago<br>
Dunedin, New Zealand</i>
51 changes: 27 additions & 24 deletions src/vector/v.surf.icw/v.surf.icw.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
#!/usr/bin/env python
#!/usr/bin/env python3
#############################################################################
#
# MODULE: v.surf.icw
# version $Id$
#
# AUTHOR: M. Hamish Bowman, Dunedin, New Zealand
# Originally written aboard the NZ DoC ship M/V Renown,
Expand All @@ -13,7 +12,7 @@
# PURPOSE: Like IDW interpolation, but distance is cost to get to any
# other site.
#
# COPYRIGHT: (c) 2003-2014 Hamish Bowman
# COPYRIGHT: (c) 2003-2024 Hamish Bowman
# This program is free software under the GNU General Public
# License (>=v2). Read the file COPYING that comes with GRASS
# for details.
Expand Down Expand Up @@ -135,10 +134,12 @@
def cleanup():
grass.verbose(_("Cleanup.."))
tmp_base = "tmp_icw_" + str(os.getpid()) + "_"
grass.run_command(
"g.remove", flags="f", type="raster", pattern=tmp_base + "*", quiet=True
)
grass.try_remove(TMP_FILE)
# grass.run_command('g.list', type='raster', mapset='.', flags='p')
result = grass.list_strings('raster', pattern=tmp_base + "*", mapset='.')
if len(result) > 0:
grass.run_command(
"g.remove", flags="f", type="raster", pattern=tmp_base + "*", quiet=True
)


def main():
Expand Down Expand Up @@ -345,7 +346,7 @@ def main():
)
# stall to wait for the nth worker to complete,
if (i + 1) % workers == 0:
# print 'stalling ...'
# print('stalling ...')
proc[i].wait()

# make sure everyone is finished
Expand Down Expand Up @@ -393,7 +394,7 @@ def main():
)
# stall to wait for the nth worker to complete,
if (i + 1) % workers == 0:
# print 'stalling ...'
# print('stalling ...')
proc[i].wait()

# r.patch in=1by_cost_site_sqrd.${NUM},tmp_idw_cost_val_$$ out=1by_cost_site_sqrd.${NUM} --o
Expand All @@ -415,8 +416,7 @@ def main():

#######################################################
#### Step 3) find sum(cost^2)
grass.verbose("")
grass.verbose(_("Finding sum of squares ..."))
grass.verbose("\n" + _("Finding sum of squares ..."))

# todo: test if MASK exists already, fatal exit if it does?
if post_mask:
Expand All @@ -425,14 +425,12 @@ def main():

grass.message(_("Summation of cost weights ..."))

input_maps = tmp_base + "1by_cost_site_sq.%05d" % 1

global TMP_FILE
TMP_FILE = grass.tempfile()
with open(TMP_FILE, "w") as maplist:
for i in range(2, n + 1):
for i in range(1, n + 1):
mapname = "%s1by_cost_site_sq.%05d" % (tmp_base, i)
maplist.write(mapname + "\n")
maplist.close()

# grass.run_command('g.list', type = 'raster', mapset = '.')

Expand All @@ -444,14 +442,15 @@ def main():
except CalledModuleError:
grass.fatal(_("Problem running %s") % "r.series")

grass.try_remove(TMP_FILE)

if post_mask:
grass.message(_("Removing post_mask <%s>"), post_mask)
grass.run_command("g.remove", flags="f", name="MASK", quiet=True)

#######################################################
#### Step 4) ( 1/di^2 / sum(1/d^2) ) * ai
grass.verbose("")
grass.message(_("Creating partial weights ..."))
grass.message("\n" + _("Creating partial weights ..."))

proc = {}
num = 1
Expand Down Expand Up @@ -534,23 +533,27 @@ def main():
# grass.run_command('g.list', type = 'raster', mapset = '.')

#######################################################
grass.message("")
grass.message(_("Calculating final values ..."))
grass.message("\n" + _("Calculating final values ..."))

input_maps = tmp_base + "partial.%05d" % 1
for i in range(2, n + 1):
input_maps += ",%spartial.%05d" % (tmp_base, i)
TMP_FILE = grass.tempfile()
with open(TMP_FILE, "w") as maplist:
Copy link
Contributor

Choose a reason for hiding this comment

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

See https://github.com/OSGeo/grass/blob/main/doc/development/style_guide.md#temporary-files

g.tempfile is meant for large data, in this case standard system tempfile seems better.

Copy link
Contributor

Choose a reason for hiding this comment

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

See also how to deal with temporary maps in the guide. Right now you call cleanup function explicitly, but does it get called again since it's registered with atexit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See also how to deal with temporary maps in the guide. Right now you
call cleanup function explicitly, but does it get called again since it's
registered with atexit?

It gets called on exit a second time, but only tries to remove if something actually exists. It could be improved, tmp_base should be a global variable for one thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See https://github.com/OSGeo/grass/blob/main/doc/development/style_guide.md#temporary-files

g.tempfile is meant for large data, in this case standard system tempfile seems better.

Any particular reason why? I don't really mind either way but g.tempfile only takes 1.36 milliseconds to complete for me, is only run twice during a long running module, doesn't pollute the filesystem outside of the mapset's .tmp dir, and gets a second chance to be cleared when the GRASS session ends/launches if the original core.try_remove() fails for some reason. g.tempfile just tries to create the file and reports back what its name is, it doesn't know or care about what ends up in it. For sure large files should use g.tempfile so that the disk space is used in MAPSET and we don't risk filling up a small /tmp partition.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, g.tempfile is meant for large files and also for Bash/shells, but otherwise there is little reason to use it. 1) In general, relying on a standard library, in this case Python standard library, is better than relying on our custom mechanism. The library provides standardized, well-document and well-tested tools with known limits. 2) Security concerns are known and addressed. 3) Code analyzers can understand the code better. 4) Other readers need to know only standard Python, not GRASS GIS specific API. 5) Creating files in the mapset can be significantly slower than creating them in system's tmp dir. That dir is meant to be fast, that's at least what most application would expect. /tmp is sometimes mounted to memory. On the other hand, mapset dir may be on a slow network drive for storage of GRASS projects, large data, or because of parallel, multi-node processing in the same GRASS project.

Thinking about this more, some Python functions for tmp files and dirs allow you to pick the base tmp dir, so you can use the standard Python functions with mapset's .tmp if large files are a concern. (I used a custom dir approach to implement a temporary mapset session (TemporaryMapsetSession, create_temporary_mapset).) If that seems like a good way, we may want to document that g.tempfile is meant only for Bash/shells scripts and perhaps add a convenient wrapper for Python functions to work over mapset's tmp dir.

Another take on the situation is that the standard Python functions (and related code quality checks) are designed in a way that the code should be sure everything is properly deleted at the right time. The with statement is the prime example of that and that's what is usually appropriate and sufficient in individual scripts or tools (it is more difficult in the library where things should to be designed to work with the with statement, but may not be able use it themselves). This is different from the older approach of deleting on couple different levels or places in case the previous delete didn't work out or was forgotten.

for i in range(1, n + 1):
mapname = "%spartial.%05d" % (tmp_base, i)
maplist.write(mapname + "\n")
maplist.close()

try:
grass.run_command("r.series", method="sum", input=input_maps, output=output)
grass.run_command("r.series", method="sum", file=TMP_FILE, output=output)
except CalledModuleError:
grass.fatal(_("Problem running %s") % "r.series")

grass.try_remove(TMP_FILE)

# TODO: r.patch in v.to.rast of values at exact seed site locations. currently set to null

grass.run_command("r.colors", map=output, color="bcyr", quiet=True)
grass.run_command(
"r.support", map=output, history="", title="Inverse cost-weighted interpolation"
"r.support", map=output, history=" ", title="Inverse cost-weighted interpolation"
)
grass.run_command("r.support", map=output, history="v.surf.icw interpolation:")
grass.run_command(
Expand Down
Loading