GeoJSON: expose field names in a more consistent orders when some feature lacks some fields (refs qgis/QGIS#45139) #4552
Conversation
…ture lacks some fields (refs qgis/QGIS#45139)
rouault
force-pushed
the
fix_qgis_45139
branch
from
a3027e4 to
f28f5e8
Compare
|
|
||
| if( isTherePathFromTo(j, i) ) | ||
| { | ||
| return "can't add edge: this would cause a cycle"; |
There was a problem hiding this comment.
should you drop both instead and sort these two by some other means? this line will keep the attribute order feature-order-dependant still.
There was a problem hiding this comment.
good point, but what would you drop if we have edges A->B and B->C, and we now want to insert C->A ?
There was a problem hiding this comment.
all three as they can't be treated as definitions? but you can't do that on this same pass as then next feature will push them back. you generally need to build cyclic graph and then cut off cycles explicitly.
other strategies that come to mind:
- push the edges into a set first, order it alphabetically as tuples, push into current dag. will keep alphabetical ordering in case of conflicts.
- count number of occurences for each edge, sort by that then alphabetically, push into current dag. will keep most common order in case of conflicts.
- say "it will be this way" and do nothing, maybe put a note. current one still works. :)
There was a problem hiding this comment.
I guess first option can also be simplified to edge with from < to beats all conflicting edges with from > to. So in your example C->A is dropped in any order of insertion.
There was a problem hiding this comment.
I believe I will keep suggested improvements (thanks for them) for a next time. Conflicting order is a less common case, and cannot be generated with GDAL.
| for( int idx: anCurFieldIndices ) | ||
| { | ||
| dag.addNode(idx, apoFieldDefn[idx]->GetNameRef()); | ||
| if( nPrevFieldIdx != -1 ) |
There was a problem hiding this comment.
if you ever decide to do edge-counting, the n^2 loop with all pairs and not just adjacent neighbors will get more stable voting.
There was a problem hiding this comment.
I've added a commit with a comment in the code pointing to that PR if we need those enhancements
…e of conflicting order [ci skip]
Detailed rationale of the fix: http://erouault.blogspot.com/2021/09/order-of-geojson-properties-and-graph.html