-
-
Notifications
You must be signed in to change notification settings - Fork 295
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
Integrate band references into portable signature files #1501
Conversation
|
(I accidentally switched the checkboxes, didn't realise there is no need to start "editing", however I did reset it as it was. Sorry!) |
3d52567
to
9fe79bb
Compare
|
The amount of changes will shrink after merge of #1272 (it will be easier to do a review after that has been done). |
b6265c9
to
bf067cd
Compare
* as band references are used only for raster, move functions to raster lib * add function to test basic band id naming conformance * add tests
…id also without extended metadata
…tum metadata As file name can be determined at runtime and is used only to print out extended metadata, it makes little sense to store it. This greatly simplifies work with band references.
* as band references are used only for raster, move functions to raster lib * add function to test basic band id naming conformance * add tests
…tum metadata As file name can be determined at runtime and is used only to print out extended metadata, it makes little sense to store it. This greatly simplifies work with band references.
* write band references to a signature file for later band - raster mapping; * move signatures out of imagery groups to enable signature file reuse; * add tests for signature file writing and reading.
|
Anyone willing to review the code? |
Unfortunately I have not got time to get to it yet. I hope to take a look next week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First, the tests look great. What a great example of how heavy C can be tested.
I looked mostly at the new mapset structure additions. signatures/signature_type/{name} seems to be a totally new kind of thing in mapset structure (or?), so I think some library additions and further considerations are needed.
If you provide some examples (or point to existing ones) people can now try in Binder without risk of messing up their mapsets. The link is https://mybinder.org/v2/gh/marisn/grass/imsigs?urlpath=lab%2Ftree%2Fdoc%2Fnotebooks%2Fexample_notebook.ipynb
I have two additional questions. Does the band reference naming fit into these new signature files? (This goes back to getting the band reference (image collection?) naming right.) Does this totally remove signatures from groups (or is there still some relation)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bunch of changes, additions, a lot to look into. The present comments are the result of my first pass.
| fq_name = "{}@{}".format(self.sig_name, self.mapset_name) | ||
| p_old_sigfile = I_fopen_sigset_file_old(fq_name) | ||
| ret = I_ReadSigSet(p_old_sigfile, ctypes.byref(Sn)) | ||
| self.assertEqual(ret, 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running on Mac gives:
FAIL: test_roundtrip_sigset_v1_one_band (__main__.SigSetFileTestCase)
Test writing and reading back sigset file (v1)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Volumes/dev/grass/lib/imagery/testsuite/test_imagery_sigsetfile.py", line 98, in test_roundtrip_sigset_v1_one_band
self.assertEqual(ret, 1)
AssertionError: -1 != 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As there where memory handling related changes, please re-run the test.
| Sn = SigSet() | ||
| p_old_sigfile = I_fopen_sigset_file_old(self.sig_name) | ||
| ret = I_ReadSigSet(p_old_sigfile, ctypes.byref(Sn)) | ||
| self.assertEqual(ret, 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running on Mac gives:
FAIL: test_roundtrip_sigset_v1_two_bands (__main__.SigSetFileTestCase)
Test writing and reading back sigset (v1) with two bands
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Volumes/dev/grass/lib/imagery/testsuite/test_imagery_sigsetfile.py", line 194, in test_roundtrip_sigset_v1_two_bands
self.assertEqual(ret, 1)
AssertionError: -1 != 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As there where memory handling related changes, please re-run the test.
Getting tests right was a hard thing but it really helps to iron out various problems with code, especially if tests do memory handling well – one can run them under valgrind then.
Yes, it is a new thing in a mapset structure as signatures do not belong to rasters, vectors or groups but are on their own. Please clarify regarding "library additions and further considerations". I sent out a RFC to dev mailinglist and got only a bit of feedback.
An example in Spearfish location (warning – total nonsense!):
Band reference names can be anything as long as they allow to identify rasters containing data with identical semantic and value meaning (e.g. a single band of same satellite sensor from different days). Thus current band references are fine.
This allows to create signatures in one group and use them in any other group as long as groups are identical by meaning (=band references) and not necessary by band order or naming. Thus need for a new location for signature storage. |
|
It seems that I have lost some stuff during merging/rebase when Python was moved around as signature prompting is not operational any more. As I have force pushed after rebase, I'll have to look into GUI code again and retest all parts of code to see if nothing else has been broken (it is!). Either this slips G8 or will have to wait for G9 :( |
…ures_list_by_type()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 from the perspective of the "element" API. It has tests. So I approve for that part. The rest is comments.
It seems it lacks documentation unless I missed something or is that so obvious that the few added sentences are enough? (I mean no sarcasm, I'm asking.)
I didn't test it nor reviewed the functionality and user interface.
I see it now, but I forgot about that email and didn't make a connection - the conversation there seemed to die out (last posts I see are from Luca, Sajid and myself).
Any NC SPM examples for the manual? Or at least as a test where "non-sense" is fine. Spearfish is not readily available in Binder or CI. |
I updated a bit documentation of modules. As these changes are more internal than external, there are almost no changes for previous workflows. Only new thing is the requirement of having band references set for all rasters used for classification. The rest stays the same. With current content of NC location I do not see how to nicely expose new opportunities revealed by move to portable signature files. That would need e.g. another set of LANDSAT 7 scenes. Overall – I wouldn't call work on new signature files complete – I did all work in the library part, but a module for signature file management is still missing. Also, as you noted, would be nice to have an example displaying benefits of the new approach. But never the less, I would like to merge this ASAP to be able to move forward with 8.0 release. Missing bits can be added also later. |
Imagery: untie signature files from groups Thus far signature files have been living inside an imagery group. Mapping between individual raster maps of a group and signature values was implicit. This old design made impossible to safely re-use signatures of one imagery group to classify other group(s). The new approach is to have raster band references written inside a signature file to store mapping of signature values to rasters they represent. This change allows to safely use signature file for classification of other imagery groups as long as they contain rasters of the same semantic content (e.g. different image form the same satellite). * On signature file creation write out raster band references * On signature file reading compare group and signature file band references for a match * Signature files now live outside of groups in a dedicated folder with subfolders for each signature file type * All imagery modules are adjusted to use new signature handling functions
Imagery: untie signature files from groups Thus far signature files have been living inside an imagery group. Mapping between individual raster maps of a group and signature values was implicit. This old design made impossible to safely re-use signatures of one imagery group to classify other group(s). The new approach is to have raster band references written inside a signature file to store mapping of signature values to rasters they represent. This change allows to safely use signature file for classification of other imagery groups as long as they contain rasters of the same semantic content (e.g. different image form the same satellite). * On signature file creation write out raster band references * On signature file reading compare group and signature file band references for a match * Signature files now live outside of groups in a dedicated folder with subfolders for each signature file type * All imagery modules are adjusted to use new signature handling functions
This PR is a continuation of band reference refactoring PR #1272. It goes on the top of PR #1272.
The main aim is to create reusable signature files – create in one imagery group, use in any imagery group containing same bands. E.g. if one creates signature file for a Landsat 8 scene, he should be able to use the same signature file to classify any Landsat 8 scene.
To reach the aim, three goals must be fulfilled:
List of areas to fix: