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

v.to.db: add JSON support #4036

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

kritibirda26
Copy link
Contributor

Add JSON output support to v.to.db module. The output looks like as follows:

{
    "totals": {
        "area": 2219442027.2203522
    },
    "records": [
        {
            "category": 1,
            "area": 24375323.127803534
        },
        {
            "category": 2,
            "area": 2938964.3204806102
        },
        {
            "category": 3,
            "area": 50536154.294107705
        }
    ],
...
}

@github-actions github-actions bot added vector Related to vector data processing C Related code is in C module labels Jul 11, 2024
@kritibirda26
Copy link
Contributor Author

kritibirda26 commented Jul 25, 2024

@cwhite911 Hi! Can you please review this PR? I would like to use the JSON format added here in the v.report module's JSON implementation to keep the implementation simple.

@cwhite911
Copy link
Contributor

@cwhite911 Hi! Can you please review this PR? I would like to use the JSON format added here in the v.report module's JSON implementation to keep the implementation simple.

@kritibirda26 the schema looks good to me, however I'm debating if we should add the measurement unit to the root object.

@echoix
Copy link
Member

echoix commented Jul 27, 2024

If it's documented for now, it won't be breaking to add it at a later time if needed. So if you can't decide after thinking about it for a while, there's a way to keep the PR going.

Copy link
Contributor

@cwhite911 cwhite911 left a comment

Choose a reason for hiding this comment

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

@kritibirda26 This is looking good. Please add test for all of the possible output schemas and update the docs to make it clear that output format only works when the -p flag is set.

@github-actions github-actions bot added Python Related code is in Python HTML Related code is in HTML docs tests Related to Test Suite labels Jul 31, 2024
@kritibirda26
Copy link
Contributor Author

@cwhite911 can you please take another look at this PR?

@cwhite911
Copy link
Contributor

@kritibirda26 Please add additional unit tests to capture the all of the response data types (point, line, polygon, 3d vectors, mixed with the options: cat, area, compact, fd, perimeter, length, count, coor, start, end, sides, query, slope, sinuous, azimuth, bbox.

Let's also add the measurement unit to the root json object.

@kritibirda26
Copy link
Contributor Author

@cwhite911 Should there be at least one test covering each type and each option? asking because it doesn't seem possible to provide multiple options or types to v.to.db at once.

@cwhite911
Copy link
Contributor

@cwhite911 Should there be at least one test covering each type and each option? asking because it doesn't seem possible to provide multiple options or types to v.to.db at once.

Yes, we should add a test to make sure each key value pair is correctly generated for the option.

@kritibirda26
Copy link
Contributor Author

@cwhite911 I have added more tests and measurement units to the root of the object. Can you please suggest which maps/commands to use to test query, azimuth, sinuousity and slope?

Copy link
Member

Choose a reason for hiding this comment

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

Also make sure to avoid the uninitialized warning like the other module.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what you mean here. Will you remind me of the other module?

Copy link
Member

Choose a reason for hiding this comment

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

ff98e42,
Follow the PR, the comment, and if you have access to coverity, the new detection after the PR #4201 was merged.

Copy link
Contributor

@cwhite911 cwhite911 left a comment

Choose a reason for hiding this comment

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

The azimuth is calculated from a line feature. I suggest using the majorroads layer for the test.

Azimuth

v.to.db -p map=roadsmajor@PERMANENT option=azimuth units=radians format=json
v.to.db -p map=roadsmajor@PERMANENT option=azimuth units=degrees format=json

Sinuous

v.to.db -p map=roadsmajor@PERMANENT option=sinuous format=json

Slope

To test slope I don't think we have a layer in the test dataset the is line feature with z coordinates.

For the test you could create one using v.to.3d and then testing that with

v.to.db -p map=roadsmajor3d option=slope units=degrees format=json

@kritibirda26
Copy link
Contributor Author

@cwhite911, Hi! I have updated the PR with tests for azimuth and sinuous. For slope, I tried to create a 3d map but failed. I also followed other examples of creating a 3d dataset from 2d (from the tests of other modules) but the slope always comes to be 0 in both plain and json mode.

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 docs HTML Related code is in HTML module Python Related code is in Python tests Related to Test Suite vector Related to vector data processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants