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.gensig: Add support of original class value in sig file (for i.maxlik) #2425

Merged
merged 9 commits into from
Jun 18, 2022

Conversation

marisn
Copy link
Contributor

@marisn marisn commented Jun 10, 2022

Signature files (generated by i.gensig) contain class labels but do not contain original values from map used to generate signatures. Thus map with predicted values by i.maxlik will contain different values for same classes in comparison to training map.
This PR adds an optional ability to store original class values and apply them on to classified map to restore a complete match between training and predicted class values.

Signature files written after merging this PR are incompatible with older GRASS versions (8.0, 8.2), but older signature files (8.0, 8.2) still can be read.

@marisn marisn added this to the 8.4.0 milestone Jun 10, 2022
@marisn marisn requested a review from veroandreo June 10, 2022 14:52
@veroandreo
Copy link
Contributor

I pulled these changes and tested by digitizing a vector map with 2 classes (43 and 65, just because), converted to raster using the class number, ran i.gensig and then i.maxlik. While i.gensig uses the right number for classes, i.maxlik still renames them to 1 and 2.

@marisn marisn requested a review from wenzeslaus June 14, 2022 16:26
@marisn marisn linked an issue Jun 14, 2022 that may be closed by this pull request
@marisn marisn marked this pull request as ready for review June 14, 2022 16:27
Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

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

Although this has a test for C functions which is great, there is no higher level test for i.gensig and i.maxlik or related modules. i.maxlik has a partial example in the manual page which requires an example from i.cluster. I would be more confident if we have at least some smoke test to see that the workflows really run. Sorry, I was not following this closely, but having a test for a workflow which didn't work before and now works and also for a workflow which worked and continues to work would be great showing both what was improved and also that nothing was damaged.

include/grass/defs/imagery.h Outdated Show resolved Hide resolved
include/grass/imagery.h Show resolved Hide resolved
lib/imagery/sig.c Outdated Show resolved Hide resolved
lib/imagery/sig.c Outdated Show resolved Hide resolved
@marisn
Copy link
Contributor Author

marisn commented Jun 16, 2022

I added two tests for i.maxlik and one for i.gensig. All three tests just test successful case, there are no tests for various modes of failure.
Having a test was a really good idea as I had made an one-off error in i.maxlik code :(

@veroandreo could you give this PR another shot?

@marisn marisn requested a review from wenzeslaus June 16, 2022 11:45
imagery/i.gensig/testsuite/test_i_gensig.py Outdated Show resolved Hide resolved
lib/imagery/sig.c Outdated Show resolved Hide resolved
@marisn marisn requested a review from wenzeslaus June 16, 2022 14:25
Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

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

The new things look good.

imagery/i.gensig/testsuite/test_keyvalue_result.txt Outdated Show resolved Hide resolved
Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

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

This is good to go as far as the changed code and new tests go. I'm still little uneasy about the i.cluster -> i.maxlik workflow which is not tested anywhere.

@wenzeslaus wenzeslaus added the C Related code is in C label Jun 16, 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.

Just tested again. It works as expected, class numbers are now preserved in the result of i.maxlik

@marisn
Copy link
Contributor Author

marisn commented Jun 18, 2022

This is good to go as far as the changed code and new tests go. I'm still little uneasy about the i.cluster -> i.maxlik workflow which is not tested anywhere.

You mean i.cluster -> sig file. Writing and reading sig file is tested in lib tests and sig file -> i.maxlik is tested in i.maxlik tests.

@marisn marisn merged commit 79f9500 into OSGeo:main Jun 18, 2022
@marisn marisn deleted the sig_bug_2413 branch June 21, 2022 05:31
ninsbl pushed a commit to ninsbl/grass that referenced this pull request Oct 26, 2022
* Imagery: use original raster values for classification output in workflow i.gensig -> i.maxlik (fixes bug OSGeo#2413)
ninsbl pushed a commit to ninsbl/grass that referenced this pull request Feb 17, 2023
* Imagery: use original raster values for classification output in workflow i.gensig -> i.maxlik (fixes bug OSGeo#2413)
@wenzeslaus wenzeslaus changed the title Imagery: add support of original class value in sig file i.gensig: Add support of original class value in sig file (for i.maxlik) Jun 6, 2023
neteler pushed a commit to nilason/grass that referenced this pull request Nov 7, 2023
* Imagery: use original raster values for classification output in workflow i.gensig -> i.maxlik (fixes bug OSGeo#2413)
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] sig does not preserve original values
3 participants