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.forcircular: Evaluation of circular bioconomy level of forest ecosystems #694

Merged
merged 60 commits into from
Feb 11, 2022

Conversation

fra-lab
Copy link
Contributor

@fra-lab fra-lab commented Feb 1, 2022

Analyze and measure the level of sustanability of the forest-wood supply chain in a circular bioeconomy approach.

…nability of the forest-wood supply chain in a circular bioeconomy approach.
@petrasovaa petrasovaa added the new addon PR contains a new addon or issue proposes one label Feb 1, 2022
Copy link
Contributor

@veroandreo veroandreo left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! I did a very quick check only. Some edits and comments below.

src/raster/r.forcircular/r.forcircular.html Outdated Show resolved Hide resolved
src/raster/r.forcircular/r.forcircular Outdated Show resolved Hide resolved
src/raster/r.forcircular/r.forcircular Outdated Show resolved Hide resolved
src/raster/r.forcircular/r.forcircular Outdated Show resolved Hide resolved
src/raster/r.forcircular/r.forcircular.html Outdated Show resolved Hide resolved
src/raster/r.forcircular/r.forcircular.html Outdated Show resolved Hide resolved
src/raster/r.forcircular/r.forcircular.html Outdated Show resolved Hide resolved
src/raster/r.forcircular/r.forcircular.html Outdated Show resolved Hide resolved
src/raster/r.forcircular/r.forcircular.html Outdated Show resolved Hide resolved
@neteler neteler changed the title add new module r.forcircular r.forcircular: Evaluation of circular bioconomy level of forest ecosystems Feb 1, 2022
fra-lab and others added 8 commits February 2, 2022 09:33
Co-authored-by: Veronica Andreo <veroandreo@gmail.com>
Co-authored-by: Veronica Andreo <veroandreo@gmail.com>
Co-authored-by: Veronica Andreo <veroandreo@gmail.com>
Co-authored-by: Veronica Andreo <veroandreo@gmail.com>
Co-authored-by: Veronica Andreo <veroandreo@gmail.com>
Co-authored-by: Veronica Andreo <veroandreo@gmail.com>
Co-authored-by: Veronica Andreo <veroandreo@gmail.com>
Co-authored-by: Markus Neteler <neteler@osgeo.org>
@ninsbl
Copy link
Member

ninsbl commented Feb 2, 2022

There is some potential to make the code both more simple and more efficient. Let me know if you are interested in suggestion. Also, the hardcoded mapnames pose a risk that existing data accidentally gets overwritten or deleted. I would probably try to avoid that by using unique temporary mapnames (or prefixes).

@lucadelu
Copy link
Contributor

lucadelu commented Feb 2, 2022

There is some potential to make the code both more simple and more efficient. Let me know if you are interested in suggestion. Also, the hardcoded mapnames pose a risk that existing data accidentally gets overwritten or deleted. I would probably try to avoid that by using unique temporary mapnames (or prefixes).

I fully agree on temporary names

# for details.
#
#############################################################################
#%Module
Copy link
Contributor

Choose a reason for hiding this comment

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

Please insert a space between # and % # % in all these lines (see also #693).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

Here the spaces git lost again... please add them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@wenzeslaus
Copy link
Member

...the hardcoded mapnames pose a risk...avoid that by using unique temporary mapnames (or prefixes).

And we do have functions for in the library:

import grass.script as gs
raster_1_name = gs.append_node_pid("tmp_raster_1")
print(raster_1_name)

Copy-paste to, e.g., binder to try.

@nilason
Copy link
Contributor

nilason commented Feb 2, 2022

Watch out, you have copied the r.forcircular file to r.forcircular.py, but you should have renamed it. Now you need to keep r.forcircular.py (and delete r.forcircular), but before make sure the changes to r.forcircular are remade to r.forcircular.py.

@fra-lab
Copy link
Contributor Author

fra-lab commented Feb 10, 2022

Many many many thanks for your time and your patience!

@fra-lab
Copy link
Contributor Author

fra-lab commented Feb 10, 2022

Many many many thanks for your time and your patience!

do i have to do some other operation to close the pull request?

@nilason
Copy link
Contributor

nilason commented Feb 10, 2022

There are still minor issues in the html formatting to my taste, but they can be adressed later on, they are not so important. If there are no objections from the code point of view, I'm positive for merging this. Thanks for your contribution @fra-lab !!

From a code formality point of view, black and flake8 are happy, it seems to be good for me.

@neteler
Copy link
Member

neteler commented Feb 10, 2022

From a code formality point of view, black and flake8 are happy, it seems to be good for me.

Just the comment-spaces seem to be gone again in the parser section?

@nilason
Copy link
Contributor

nilason commented Feb 10, 2022

From a code formality point of view, black and flake8 are happy, it seems to be good for me.

Just the comment-spaces seem to be gone again in the parser section?

You're right. Missed that now. Funny, those spaces live their own life...

@fra-lab
Copy link
Contributor Author

fra-lab commented Feb 11, 2022

is there still something missing? I can't merge autonomously right? or maybe yes?

@nilason
Copy link
Contributor

nilason commented Feb 11, 2022

is there still something missing?

The spaces between #%got lost again, see #694 (comment).
Please insert them again.

I can't merge autonomously right? or maybe yes?

No, only someone with write access can merge.

@fra-lab
Copy link
Contributor Author

fra-lab commented Feb 11, 2022

is there still something missing?

The spaces between #%got lost again, see #694 (comment). Please insert them again.

I can't merge autonomously right? or maybe yes?

No, only someone with write access can merge.

Done

@neteler
Copy link
Member

neteler commented Feb 11, 2022

Congrats, thanks for your contribution. Merging now.

@neteler neteler merged commit b3111ea into OSGeo:grass8 Feb 11, 2022
@neteler
Copy link
Member

neteler commented Feb 11, 2022

@fra-lab:
For the backport to the grass7 branch, you may cherry-pick b3111ea, then remove the spaces in the parser section (G7 doesn't like them, there it needs to be #%). And then open a new pull request against the grass7 branch.

@fra-lab
Copy link
Contributor Author

fra-lab commented Feb 11, 2022

@fra-lab: For the backport to the grass7 branch, you may cherry-pick b3111ea, then remove the spaces in the parser section (G7 doesn't like them, there it needs to be #%). And then open a new pull request against the grass7 branch.

Thank you very much Markus.
I've never made a cherry pick. Could you suggest me the right git syntax?

@neteler
Copy link
Member

neteler commented Feb 11, 2022

Well, I am not a git guru at all: It is git cherry-pick b3111ea but I don't know how to get this into a new pull request on grass7 branch.

Perhaps simply open a new PR after modifying the parser space stuff and refer to this PR in the description?

@fra-lab
Copy link
Contributor Author

fra-lab commented Feb 15, 2022

On Windows 10 / Grass 8 if I try to install r.forcircular through g.extension obtain this:
g.extension extension=r.forcircular
Downloading precompiled GRASS Addons <r.forcircular>...
Fetching <r.forcircular> from http://wingrass.fsv.cvut.cz/grass80/addons/grass-8.0.0/r.forcircular.zip (be patient)...
Updating extensions metadata file...
Updating extension modules metadata file...
WARNING: No metadata available for module 'r.forcircular': Unable to fetch interface description for command '<r.forcircular>'.

Details:
Installation of <r.forcircular> successfully finished
(Tue Feb 15 16:18:38 2022) Comando terminato (11 sec)

And if I type r.forcircular both in the console and in the terminal nothing happens ..... the command is not found. Also in the extension management the addon seems not installed. But in GRASS8/addons/scripts r.forcircular.py is present.
Instead the GRASS8 / addons / bin folder is empty where (at least in version 7) the .bat file should have been.

The test was done on a Windows 10 virtual machine (vmware)

@fra-lab
Copy link
Contributor Author

fra-lab commented Feb 16, 2022

I get the same behavior with other addons (i.e. r.diversity). This is the error message:
Unable to fetch interface description for command '<r.forcircular>'. Details: <Cannot find the executable r.forcircular>

The same error message is also obtained on some core features such as r.param.scale

ScriptError : Unable to fetch interface description for command '<r.param.scale>'. Details: <C: \ Program Files \ GRASS GIS 8.0 \ extrabin \ python3.exe: can't open file 'C: \ Users \ f \ r.param.scale': [Errno 2] No such file or directory.

I tried with 2 different virtual machine and my collegue has the same beahviour with an other Windows computer.

IvanMarchesini pushed a commit to IvanMarchesini/grass-addons that referenced this pull request Feb 24, 2022
…stems (OSGeo#694)

* add new module r.forcircular: Analyze and measure the level of sustainability of the forest-wood supply chain in a circular bioeconomy approach.

* add some patch to resolve suggested issues
* change 9,999 to number format 9.999
* insert a space between # and % # %
* removed row with overwrite:yes in parse module section
* wrapped html code to 80 characters
* images aligned correctly
* accepted suggestion for wood waste; accepted suggestion for the new paragraph for the model explaination; code formatted correctly

Co-authored-by: Veronica Andreo <veroandreo@gmail.com>
Co-authored-by: Markus Neteler <neteler@osgeo.org>
Co-authored-by: Nicklas Larsson <n_larsson@yahoo.com>
@agiudiceandrea
Copy link

@fra-lab, @neteler, what is the status of the r.forcircular addon?
It seems it is impossible to install it in GRASS 8.2.0 on Windows.
On https://wingrass.fsv.cvut.cz/grass82/addons/grass-8.2.0/logs/ the compilation of the addon results failing with the following log: https://wingrass.fsv.cvut.cz/grass82/addons/grass-8.2.0/logs/r.forcircular.log.

@neteler
Copy link
Member

neteler commented Sep 29, 2022

Here the log snippet for easier consumption:

/usr/bin/install -c  r.forcircular.py /c/Users/landamar/grass_packager/grass820/addons/r.forcircular/scripts/r.forcircular.py
if [ "/c/Users/landamar/grass_packager/grass820/addons/r.forcircular/scripts/r.forcircular.py" != "" ] ; then GISRC=/usr/src/grass820/dist.x86_64-w64-mingw32/demolocation/.grassrc82 GISBASE=C:/msys64/usr/src/grass820/dist.x86_64-w64-mingw32 PATH="/usr/src/grass820/dist.x86_64-w64-mingw32/bin:/usr/src/grass820/dist.x86_64-w64-mingw32/bin:/usr/src/grass820/dist.x86_64-w64-mingw32/scripts:$PATH" PYTHONPATH="C:/msys64/usr/src/grass820/dist.x86_64-w64-mingw32/etc/python;C:/msys64/usr/src/grass820/dist.x86_64-w64-mingw32/gui/wxpython;$PYTHONPATH" PATH="/c/Users/landamar/grass_packager/grass820/addons/r.forcircular/bin:/usr/src/grass820/dist.x86_64-w64-mingw32/bin:/usr/src/grass820/dist.x86_64-w64-mingw32/scripts:/usr/src/grass820/dist.x86_64-w64-mingw32/lib:/usr/src/grass820/dist.x86_64-w64-mingw32/lib:/C/OSGeo4W/apps/qt5/bin:/C/OSGeo4W/apps/Python39/Scripts:/C/OSGeo4W/bin:/C/Windows/system32:/C/Windows:/C/Windows/system32/WBem:/usr/bin:/mingw64/bin/:/c/Users/landamar/grass_packager/grass82/addons/mswindows/osgeo4w/lib:/c/Users/landamar/grass_packager/grass82/addons/mswindows/osgeo4w:/c/windows32/system32:/c/windows:/c/windows32/system32:/c/windows:/c/msys64/usr/bin:/c/msys64/mingw64/bin" LC_ALL=C LANG=C LANGUAGE=C /c/Users/landamar/grass_packager/grass820/addons/r.forcircular/scripts/r.forcircular.py --html-description < /dev/null | grep -v '</body>\|</html>' > r.forcircular.tmp.html ; fi
Traceback (most recent call last):
  File "C:\Users\landamar\grass_packager\grass820\addons\r.forcircular\scripts\r.forcircular.py", line 395, in <module>
    from grass.pygrass.raster import RasterRow
  File "C:\msys64\usr\src\grass820\dist.x86_64-w64-mingw32\etc\python\grass\pygrass\raster\__init__.py", line 20, in <module>
    import grass.lib.raster as libraster
  File "C:\msys64\usr\src\grass820\dist.x86_64-w64-mingw32\etc\python\grass\lib\raster.py", line 30, in <module>
    _libs["grass_raster.8.2"] = load_library("grass_raster.8.2")
  File "C:\msys64\usr\src\grass820\dist.x86_64-w64-mingw32\etc\python\grass\lib\ctypes_loader.py", line 105, in __call__
    raise ImportError("Could not load %s." % libname)
ImportError: Could not load grass_raster.8.2.
make: *** [/c/msys64/usr/src/grass820/include/Make/Html.make:14: r.forcircular.tmp.html] Error 1
rm r.forcircular.tmp.html

What I am wondering, is ImportError: Could not load grass_raster.8.2. - should there be a .dll extension?
@landam @hellik, others, any idea?

@nilason
Copy link
Contributor

nilason commented Sep 29, 2022

What I am wondering, is ImportError: Could not load grass_raster.8.2. - should there be a .dll extension? @landam @hellik, others, any idea?

Not likely related to missing extension. Should be taken care of:
https://github.com/OSGeo/grass/blob/82fb7b8e93a8b87f7de827c0b1f98992759ce7a1/python/libgrass_interface_generator/ctypesgen/libraryloader.py#L373

By the look of it, all C code based addons listed in https://wingrass.fsv.cvut.cz/grass82/addons/grass-8.2.0/logs/ are failing. More probably related to build environment (PATH?).

@agiudiceandrea
Copy link

ImportError: Could not load grass_raster.8.2.

@neteler it seems it is also reported at OSGeo/grass#2288 (comment) by @tmszi.

@tmszi
Copy link
Member

tmszi commented Sep 29, 2022

By the look of it, all C code based addons listed in https://wingrass.fsv.cvut.cz/grass82/addons/grass-8.2.0/logs/ are failing. More probably related to build environment (PATH?).

Issue is completely described here OSGeo/grass#2288 (comment).

The process of building MS WinGRASS takes place on servers at CVUT university and only @landam and maybe @pesekon2 has access.

@pesekon2
Copy link
Contributor

The process of building MS WinGRASS takes place on servers at CVUT university and only @landam and maybe @pesekon2 has access.

Unfortunately, I do not have access to this one. I told @landam about the issue yesterday and he promised to take a look at that as soon as possible.

@neteler
Copy link
Member

neteler commented Oct 8, 2022

Unfortunately, I do not have access to this one. I told @landam about the issue yesterday and he promised to take a look at that as soon as possible.

@landam Do you have a chance to check this?

@agiudiceandrea
Copy link

Both https://wingrass.fsv.cvut.cz/grass82/addons/grass-8.2.0/logs/ and https://wingrass.fsv.cvut.cz/grass82/addons/grass-8.2.1/logs/ still report the fail of the r.forcircular addon build.

neteler added a commit to neteler/grass-addons that referenced this pull request Jun 1, 2023
Replace `–` with `-` to (hopefully) address
OSGeo#694 (comment)
@neteler
Copy link
Member

neteler commented Jun 1, 2023

Both https://wingrass.fsv.cvut.cz/grass82/addons/grass-8.2.0/logs/ and https://wingrass.fsv.cvut.cz/grass82/addons/grass-8.2.1/logs/ still report the fail of the r.forcircular addon build.

Fix attempted in #909

neteler added a commit that referenced this pull request Jun 1, 2023
Replace `–` with `-` to (hopefully) address
#694 (comment)
cwhite911 pushed a commit to cwhite911/grass-addons that referenced this pull request Sep 19, 2023
Replace `–` with `-` to (hopefully) address
OSGeo#694 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new addon PR contains a new addon or issue proposes one
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet