r.geomorphon: add two more comparison modes #1096
Conversation
|
before: distance_m:
NE: 169.71
N: 30.00
NW: 42.43
W: 0.00
SW: 212.13
S: 0.00
SE: 254.56
E: 390.00after: distance_m:
NE: 169.71
N: 30.00
NW: 42.43
W: 570.00
SW: 212.13
S: 30.00
SE: 254.56
E: 390.00 |
raster/r.geomorphon/pattern.c
Outdated
| * A more thorough comparison using several factors of different priority | ||
| * similar to the BGP best path selection algorithm. | ||
| */ | ||
| static int tcompare(const double na, const double za, const double nat, |
There was a problem hiding this comment.
In scompare you are using full names but here shortening them. To improve readability of code, I would suggest to unify naming of semantically identical values in both functions.
|
The new comparison function may still have some space for improvement, for example, it may take it into account the sum or an integral of a function of elevation values to distinguish between zenith and nadir better. This could be used to be less sensitive to small bumps/holes close to the centre point, and to break the tie after the distance comparison or before or even instead. (This may be easier to comprehend looking at the scribbles in my notes.) I guess, until the new comparison function settles properly it would be better not to make it the default, so the data output is consistent, even if it is not perfect. But if you think making it the default would be better, I would not mind. |
|
Rebased on the current master branch, reindented, with a more detailed commit message and renamed variables. |
|
(Sidenote: the Windows test currently fails due to wingrass server down @landam can you pls check it?) |
This sounds like a possibility of yet another comparison function. Would it make sense to make it an key-value option instead of a flag? An option may make it easier to introduce more changes later like changing the default. (We also had the key-value options over flags discussion at the virtual sprint.) |
|
Let me explain this in a bit more detail. The elevation data I use had triggered an otherwise rare edge case, which made me look into the method and recognize it as a valid application of a multiple factor comparison. The new function does solve the edge case I had reproduced, and, as far as possible to tell from the code, two other edge cases (as explained in the commit message). It is a step in the right direction, but not necessarily the final destination. From some prior experience in this area (natural sort and routing protocols) I know that when multiple factors become involved in a comparison, it takes time to figure it out which specific factors are meaningful, how they combine and what weight/priority they have. So the best comparison function available in a year time may be different from the best function now. Whether the best function should be the default is a matter of user expectations (the same input results in the same output). A major release could be a good time to change the default function, although in my use case this detail is not essential so I would be fine either way. Removing the old function would make it impossible to reproduce old results in new versions of GRASS, which would be very inconvenient for anyone in the middle of or after a long research, so let's not do that, at least not immediately. Considering that and the point of using an option instead of a flag, let me suggest in the next revision to make the current behaviour |
|
I agree with all/most of your points @infrastation |
|
Fine, let it be underscore. Maris, do you have any input that should be taken into account in the next revision of this change? |
|
Whilst working on the next revision, I have discovered more subtleties that need to be addressed in code and comments, this will hopefully be ready in one or two days. |
The geomorphon computation includes a procedure done for each of the eight cardinal directions. The procedure traces through the elevation data to find the angles of zenith and nadir line of sight and to decide if the cardinal point is above (1), below (-1) or level (0) with the central point. The existing nadir/zenith comparison defaults to 0 when zenith and nadir angles are exactly equal and at least one angle is over its threshold. For example, this would be the case if the zenith point was 20 raster cells away at height +20m and the nadir point was 1 raster cell away at height -1m. This is much more likely to happen when the elevation data is low-resolution. Specifically, at 30m raster resolution and 1m vertical resolution this happened every 1 out of 25 cases on average. In that edge case the computed distance to the cardinal point remains 0, which results in incorrect derived cartesian coordinates and related results, most notably azimuth and elongation. Besides that, the comparison does not specifically check that when it comes to a 1 or -1 result, the corresponding angle is above its threshold. Address these issues by introducing a new "comparison" command-line option, in which "anglev1" stands for the exact existing behaviour and "anglev2" and "anglev2_distance" enable more sophisticated comparison methods.
infrastation
changed the title
|
Having explained the reasoning behind additional comparison modes, I went to check again that the traditional behaviour remains available as one of the options, and had realized that was not actually the case. This new revision gets that right by means of three modes instead of two, which makes more sense of the parameter instead of a flag. To put the following explanations into context, please mind I compare the results using two extra layers of code: one that saves the geomorphon profile data (it looks ready for a pull request after this one) and another that visualizes the profile data (this is being finished), so changes to individual geomorphon shapes are immediately apparent in the project I am working on. The octagon dis-figuration in the earlier illustration was caused by me not realizing there was an edge case. It was a temporary effect, which had exaggerated the underlying issue and led me to study the actual issue. In the new revision if the comparison fails to set a cardinal direction distance to a non-zero value, the cardinal point ends up at the geomorphon centre. For clarity, the edge case was as follows: The exact traditional comparison is now available as "comparison=anglev1" (the default). A new mode is "comparison=anglev2". At raster output level it makes a visible effect (top = before, bottom = after) on "azimuth" (greenish) and "elongation" (dark violet) outputs.
There is a very small visual effect on the "forms" output, I could tell only a few pixels different (which looks about right, because at 1/25 faulty geomorphon rate, when only one of the eight cardinal directions goes wrong, it is about a 1/5 probability of a different final landform category, so about 1 in 125 output pixels would be different). But once again, for an individual geomorphon the difference can be very apparent. My test set is 430 POIs. As far as apparent visual difference goes, the change from "anglev1" to "anglev2" had fully repaired two and partly improved three geomorphon shapes, for example (full repair):
Another new mode is "comparison=anglev2_distance". At raster output level it makes no visual difference. For individual geomorphons it had fully repaired 7 and improved 8 geomorphon shapes in the same test set, for example (partial repair):
That said, there is still plenty of space for improvement: quite a few geomorphon shapes do not make visual sense for the terrain where they are computed. This may have to do with the computation parameter values, or other edge cases in need of other comparison improvements, which I might or might not have the time to make later. Or maybe both. The approaching deadline of my user's research project reminds me that "working is better than perfect", so I am fine to make this much improvement in this specific area and to progress to getting the next layer of code reviewed and merged. Please review. |
There was a problem hiding this comment.
The motivation for the change is well documented in the PR, the code and comments explain what is happening, and the basic forms output is same on NC SPM elevation raster before and after the change. (The current automatic test only test presence of categories and it runs fine, too.)
par_comparison->options in this case needs par_comparison->descriptions because the values are not obvious, but it is well documented in the manual page, so I'm leaving this for future work.
I'm sorry for the wait. I hope it was not too inconvenient given the subsequent PR. Merging now.
|
Thank you very much. |
This gets some more data right in my project, see the attached image for an example.
