Conversation
petrasovaa
left a comment
There was a problem hiding this comment.
Please use pre-commit.
vector/v.net/report.c
Outdated
| struct boxlist *List; | ||
| G_JSON_Value *root_value = NULL; | ||
| G_JSON_Array *root_array = NULL; | ||
| if (format && strcmp(format, "json") == 0) { |
There was a problem hiding this comment.
See the other tools with json support, they use enums.
vector/v.net/v.net.md
Outdated
| ] | ||
| ``` | ||
|
|
||
| ### Produce Report in JSON format |
There was a problem hiding this comment.
To be consistent with the other examples, use nc_spm_08_grass7 dataset. Consider moving the json example more towards the top where the example is already using report.
|
I added a check for G_OPT_F_FORMAT in args.c because it was causing a segmentation fault and UI warnings on my environment when the description was missing. |
| opt->format->key = "format"; | ||
| if (!opt->format->description) | ||
| opt->format->description = _("Output format"); |
There was a problem hiding this comment.
There should not be any need to add these.
| opt->format->key = "format"; | |
| if (!opt->format->description) | |
| opt->format->description = _("Output format"); |
There was a problem hiding this comment.
"After removing those parts and running make, the build gets stuck at the UI description generation step with the following warnings: 'Missing option key' and 'Description for option <?> missing'.
There was a problem hiding this comment.
I tested it and it works fine for me. Look at other usages of G_OPT_F_FORMAT.
There was a problem hiding this comment.
Maybe you'd need to clean before make, like make clean, make libsclean, or make distclean. With the last one, you'd need to run configure again
vector/v.net/report.c
Outdated
| nnodes, x, y, z, cat_line); | ||
| } | ||
| fprintf(stdout, "%d %d %d\n", cat_line, cat_node[0], cat_node[1]); | ||
| if (root_array) { |
There was a problem hiding this comment.
It would be more readable to test the format enum value than root_array
|
Hi @petrasovaa , just wanted to check if you had a chance to look at this PR when you have a moment. Thanks! |
vector/v.net/report.c
Outdated
| G_JSON_Value *lines_val = | ||
| root_arr ? G_json_value_init_array() : NULL; | ||
|
|
||
| if (root_arr) { |
There was a problem hiding this comment.
My previous comment about testing the enum instead of root_array applies here as well and check other places.
vector/v.net/Makefile
Outdated
| LIBES = $(VECTORLIB) $(GISLIB) $(DBMILIB) | ||
| DEPENDENCIES = $(VECTORDEP) $(GISDEP) $(DBMIDEP) | ||
| LIBES = $(VECTORLIB) $(GISLIB) $(DBMILIB) $(PARSONLIB) | ||
| DEPENDENCIES = $(VECTORDEP) $(GISDEP) $(DBMIDEP) $(PARSONDEP) |
There was a problem hiding this comment.
The other Makefiles don't add PARSONDEP, I am honestly not sure about this, but to keep it consistent, I would remove this.
|
Was it closed by mistake? |
|
Reviewing is hard when force merged are made after there was a first review. Not only we Connor just read where we left off, but comments posted are on commits (and lines) that don't exist anymore, and are now kind of "detached" from the PR changed files view (and commits view) |
|
The commit 72dd6b2 seems affected by a bad rebase, its tricky when we force-push. |
Hi!, yes you are right I opened a clean new branch and I will pull request it from there today.. thank you!. |
|
We didn't review more since, you might as well fix up the same branch, so we still have the context of what we already discussed (instead of having it lost in a new PR) |
|
@echoix I've verified the changes locally with super-linter. Could you please approve the workflows to run the CI tests? |
vector/v.net/testsuite/test_v_net.py
Outdated
| """Verify that v.net report produces valid and accurate JSON output""" | ||
| vnet_module = SimpleModule( | ||
| "v.net", | ||
| input="streets_net", |
There was a problem hiding this comment.
Where is this found? It doesn't seem to be in the test dataset, or created just for the test, as the same tests fail on all platforms
There was a problem hiding this comment.
You are right, streets_net was part of my local North Carolina sample dataset and I mistakenly assumed it was available in the test environment. I will update the test to either use a standard test dataset.Sorry for the oversight!
There was a problem hiding this comment.
Could you double check which of the tests take long? In the error logs, these 6 tests in the file took 69 seconds to run on Ubuntu, which is quite long in my opinion. Maybe they were already long, but take a look to see if it is the newly added tests.
About 38.4sec on macOS (the Apple silicon chips are really fast, so it's normal, about 2x as fast as the Linux runners usually).
66 sec for the Linux CMake
Windows didn't finish run it yet.
There was a problem hiding this comment.
I've optimized the tests to address the execution time concerns. The v.net network preparation steps are now moved to setUpClass, so they only run once for the entire test suite. My local tests show a significant reduction in total runtime. I've also ensured that existing tests and the tearDown logic remain intact.
There was a problem hiding this comment.
What was the before and after (approx), and what was the other tests to compare to?
There was a problem hiding this comment.
I realized I was preparing the heavy streets dataset twice (once for each new test). I have now moved the network preparation to setUpClass so it only runs once. This fixed the slowdown. Before it was 38 sec. in my local mac now it is 28 sec. in my local mac.
This PR adds JSON output support to the v.net module using the gjson/parson library.
Description
This PR introduces JSON output support for the
v.netmodule, specifically for theoperation=nodestask. This enhancement allows users to programmatically capture network statistics (such as new node counts) for use in automated workflows and testing pipelines.Changes
G_JSONsupport inmain.cto handle structured output.formatparameter to choose betweenplain(default) andjson.v.net.mdwith usage examples and guidelines for JSON output.testsuite/test_v_net.pyto ensure output consistency.How to Test
To verify the implementation, you can use the North Carolina sample dataset.
Expected JSON Output :
[
{
"line_cat": 49599,
"start_node_cat": -1,
"end_node_cat": -1
},
...
]
Copying features...
100%
Building topology for vector map test_vnet@PERMANENT...
Registering primitives...
40000
Copying attributes...
Building topology for vector map test_vnet@PERMANENT...
Registering primitives...
50000
v.net complete. 323 lines (network arcs) written to output.
.
Copying features...
100%
Building topology for vector map test_vnet@PERMANENT...
Registering primitives...
40000
Copying attributes...
Building topology for vector map test_vnet@PERMANENT...
Registering primitives...
50000
v.net complete. 156 lines (network arcs) written to output.
.
Copying attributes...
Building topology for vector map test_vnet@PERMANENT...
Registering primitives...
90000
v.net complete. 41813 new points (nodes) written to output.
.v.net complete.
.v.net complete.
.OK