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

i.svm: Add libsvm-based image classification #2189

Merged
merged 142 commits into from
Jan 9, 2024
Merged

i.svm: Add libsvm-based image classification #2189

merged 142 commits into from
Jan 9, 2024

Conversation

marisn
Copy link
Contributor

@marisn marisn commented Feb 11, 2022

A pair of modules revealing most of features of libsvm to GRASS.
Libsvm is a popular library used in many machine learning programs (including scikit-learn). Contrary to some add-ons, this is a pure C implementation thus bypassing Python and depending on presence of libsvm.

Missing features:

  • ability to use a precomputed kernel
  • ability to provide weights for classifying unbalanced data
  • parallel processing for value prediction/estimation

This is still a WIP as some rough edges still need to be ironed out.

…ructure (a.k.a. _misc)

Full path to signature file now is:
location/mapset/signatures/<type>/<name>/<element>

This new structure will allow to store arbitrary number of files for each signature.
None of existing signature types utilizes the new functionality.
As libsvm signatures will consist of multiple files, modification to management functions was necessary.
* use the new portable signature file layout in the mapset
* add simple tests
* add documentation
@marisn marisn added this to the 8.2.0 milestone Feb 11, 2022
@wenzeslaus
Copy link
Member

I clicked the Update branch (with merge) to see if this passes after #2281 (see comment from @echoix).

@github-actions github-actions bot added GUI wxGUI related Python Related code is in Python C Related code is in C HTML Related code is in HTML libraries module and removed raster Related to raster data processing labels Jan 3, 2024
@nilason
Copy link
Contributor

nilason commented Jan 3, 2024

I clicked the Update branch (with merge) to see if this passes after #2281 (see comment from @echoix).

configure need to be regenerated, now with autoconf 2.71.

@wenzeslaus
Copy link
Member

configure need to be regenerated, now with autoconf 2.71.

I don't like that neither Git nor CI see that issue. (I don't either, but that's an expected behavior.)

@echoix
Copy link
Member

echoix commented Jan 5, 2024

I clicked the Update branch (with merge) to see if this passes after #2281 (see comment from @echoix).

configure need to be regenerated, now with autoconf 2.71.

My initial implementation of the periodic update workflow in #3200 was explicitly checking that, by regenerating configure each month, no make sure nothing changes, a bit like what the existing procedure in the docs were mentioning. But it got reviewed down to simply updating the config.guess and config.sub, and nothing more, since it was thought that the ubuntu version of autoconf might be too customized and if it was the case, the output could be slightly different. But when done manually, is autoconf really compiled from source just for that?

In either case, if its now autoconf 2.71 that is used, we might have to update the dependency listed in here

Copy link
Contributor

@nilason nilason left a comment

Choose a reason for hiding this comment

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

Sorry, but here we go again, also with 2.71.

configure Outdated Show resolved Hide resolved
configure Outdated Show resolved Hide resolved
@echoix
Copy link
Member

echoix commented Jan 8, 2024

In Ubuntu, that patch seems to be in 2.73-3, and not in 2.73-2, and Ubuntu Jammy (22.04) didn't get 2.73-3, stays at 2.73-2.

We must decide which type of system is the source of truth, and all other systems should rerun autoconf (like they should, committing configure as a generated file isn't the best recommandation except for releases) to have the appropriate patches for their system.

And is there a real issue with that makes the software unbuildable or incorrect with this ? The patches seem to originate from a bug that the packagers had to patch for.

@nilason
Copy link
Contributor

nilason commented Jan 8, 2024

In Ubuntu, that patch seems to be in 2.73-3, and not in 2.73-2, and Ubuntu Jammy (22.04) didn't get 2.73-3, stays at 2.73-2.

We must decide which type of system is the source of truth, and all other systems should rerun autoconf (like they should, committing configure as a generated file isn't the best recommandation except for releases) to have the appropriate patches for their system.

And is there a real issue with that makes the software unbuildable or incorrect with this ? The patches seem to originate from a bug that the packagers had to patch for.

As long as we pre-generate configure it has to be done with a patch-free version (easy to compile if needed), to avoid unrelated back-and-forth changes. If absolutely needed by packaging it may be re-generated on-the-fly.

@marisn
Copy link
Contributor Author

marisn commented Jan 9, 2024

As long as we pre-generate configure it has to be done with a patch-free version (easy to compile if needed), to avoid unrelated back-and-forth changes. If absolutely needed by packaging it may be re-generated on-the-fly.

This is problematic as I even had no idea I have some kind of "special" version that should not be used. Besides, as a former Gentoo user, I got used to run patched versions of packages ;-)

@nilason
Copy link
Contributor

nilason commented Jan 9, 2024

As long as we pre-generate configure it has to be done with a patch-free version (easy to compile if needed), to avoid unrelated back-and-forth changes. If absolutely needed by packaging it may be re-generated on-the-fly.

This is problematic as I even had no idea I have some kind of "special" version that should not be used. Besides, as a former Gentoo user, I got used to run patched versions of packages ;-)

I understand it is an annoyance. The changes I noted are, by the way, included with autoconf 2.72 and as far as I can tell not critical at all for us. For this PR it is enough to revert the two lines, no need to generate.

@nilason
Copy link
Contributor

nilason commented Jan 9, 2024

Great, do hurry up to merge as soon as the CIs are done! :)

@echoix echoix enabled auto-merge (squash) January 9, 2024 11:30
@nilason nilason disabled auto-merge January 9, 2024 11:36
@nilason
Copy link
Contributor

nilason commented Jan 9, 2024

@echoix Better leave the merge to @marisn .

@nilason
Copy link
Contributor

nilason commented Jan 9, 2024

@echoix Better leave the merge to @marisn .

He is the author and assignee of this PR.

@nilason
Copy link
Contributor

nilason commented Jan 9, 2024

@echoix Better leave the merge to @marisn .

He is the author and assignee of this PR.

Thinking about it, please, do not activate auto-merge at all: with squash the commit message more often than not need to be refined.

@echoix
Copy link
Member

echoix commented Jan 9, 2024

Thinking about it, please, do not activate auto-merge at all: with squash the commit message more often than not need to be refined.

In what other cases would you hope to tune the squash message (to know when it applies)? You have the box to edit it when using that option, it shows the same thing as normally, but earlier instead of later.

@nilason
Copy link
Contributor

nilason commented Jan 9, 2024

Thinking about it, please, do not activate auto-merge at all: with squash the commit message more often than not need to be refined.

In what other cases would you hope to tune the squash message (to know when it applies)? You have the box to edit it when using that option, it shows the same thing as normally, but earlier instead of later.

List is long, but not every commit message in a PR is a candidate for merge commit message, some obvious ones: "fix typo", "apply black", "revert thisorthat"; nor any rebase/merge on main (I noticed if someone hits the "update branch", that person is also added as co-author, which is not a good reason for co-authorship).
It should be a clear summary of the purpose of the final commit, which is the job for the assignee to compose.

@marisn marisn merged commit fdc72d1 into OSGeo:main Jan 9, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C Related code is in C enhancement New feature or request GUI wxGUI related HTML Related code is in HTML libraries module Python Related code is in Python
Development

Successfully merging this pull request may close these issues.