-
-
Notifications
You must be signed in to change notification settings - Fork 313
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.edit: Implement the batch option for batch editing #3450
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Martin Landa <[email protected]>
Co-authored-by: Martin Landa <[email protected]>
Co-authored-by: Martin Landa <[email protected]>
…the batch table or do not make sense
…the batch table or do not make sense
This is for a separate PR? Lines 473 to 478 in 87d2d42
I think we need |
Please try it again. |
I was wrong. Level 2 vector always requires topology building to remain level 2 even if there are no changes. Just tried this: Vect_open_update2();
/* do nothing */
Vect_close(); Level 2 vector became level 1. I'm not sure why and how... |
Please try #3459. If this works for you, you are welcome to include this single line in your PR and I close mine without merging. |
Now with support for multiple batch tables in one input, I don't see a strong need for JSON implementation in the module. I still think multiple formats should be supported more generally outside individual modules. The new option at least supports a subset1 of a well-documented version of CSV and Python users should be able to use the csv module. Footnotes
|
Thanks for the PR. With that change, |
Should be fixed with the latest commit to #3459. In case of a crash, the original topo info is (should be) preserved. |
Can you please add a test? Both grass.gunittest and pytest would work here. pytest is now little easier than before because grass.script.create_location() does not require an existing session. grass.gunittest would be needed if you want to turn your example into a test. Python doc is quite good for CSV writing, but here is an example of CSV writer from a Python dictionary I recently wrote. |
I'll do that when I get a chance. |
Tested successfully. |
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.
From my point of view, this PR may be merged when a new test for a batch
option will be also added (and required CI tests will pass)
@HuidaeCho Can we manage for 8.4.0 or will be postpone for 8.5? It is a new feature so I am not sure if it's a good idea to introduce it in 8.4.1 (?) |
This PR needs to wait for 8.5. It is failing existing unrelated tests which just happen to use This also reveals the issue that there are no tests for the current functionality. These can be added in a separate PR. |
This PR implements the new batch option for batch editing of a lot of features. For example, without this option, we need this script to extract the upper halves of
roadsmajor
from the NC sample dataset,It took 11.938 seconds on i9-12900. On the same CPU, with the new option, it took 2.180 seconds:
Red is the upper halves.