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.

Sign up for GitHub

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

Jump to bottom

introduce a vector dataset writer #315

Merged
merged 13 commits into from
Aug 18, 2022
Merged

introduce a vector dataset writer #315

merged 13 commits into from
Aug 18, 2022

Conversation

maxfreu
Copy link
Contributor

@maxfreu maxfreu commented Aug 5, 2022

Hi! Here comes the vector dataset writer. It still is far from perfect and needs tests, but you can already take a look.

yeesian
yeesian reviewed Aug 6, 2022
View reviewed changes
Copy link
Owner

@yeesian yeesian left a comment

Choose a reason for hiding this comment

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

Just leaving some thoughts while it's a draft, thank you for digging into it!

src/ArchGDAL.jl Outdated Show resolved Hide resolved
src/dataset.jl Outdated Show resolved Hide resolved
src/dataset.jl Outdated Show resolved Hide resolved
src/dataset.jl Outdated Show resolved Hide resolved
maxfreu
maxfreu commented Aug 8, 2022
View reviewed changes
src/ogr/featuredefn.jl Outdated Show resolved Hide resolved
maxfreu
maxfreu commented Aug 9, 2022
View reviewed changes
src/dataset.jl Outdated Show resolved Hide resolved
maxfreu
maxfreu commented Aug 9, 2022
View reviewed changes
src/dataset.jl Outdated Show resolved Hide resolved
maxfreu
maxfreu commented Aug 9, 2022
View reviewed changes
src/dataset.jl Show resolved Hide resolved
maxfreu
maxfreu commented Aug 9, 2022
View reviewed changes
src/dataset.jl Outdated Show resolved Hide resolved
maxfreu
maxfreu commented Aug 9, 2022
View reviewed changes
src/dataset.jl Show resolved Hide resolved
maxfreu
maxfreu commented Aug 9, 2022
View reviewed changes
src/dataset.jl Outdated Show resolved Hide resolved
maxfreu
maxfreu commented Aug 9, 2022
View reviewed changes
test/test_dataset.jl Outdated Show resolved Hide resolved
@maxfreu
Copy link
Contributor Author

maxfreu commented Aug 9, 2022

Seems like some tests are still failing on mac and windows, maybe I have to remove some drivers from the test and rewrite the test a bit.

yeesian
yeesian reviewed Aug 10, 2022
View reviewed changes
Copy link
Owner

@yeesian yeesian left a comment

Choose a reason for hiding this comment

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

I really appreciate the effort here.

In case it's lost among the comments: just wanted to make it clear that I think a single write function that "does the right thing" (i.e. writes to file the way a GDAL user would like to) is my preferred approach here. I haven't seen a compelling reason to have separate writerasters and writevectors function.

src/dataset.jl Show resolved Hide resolved
src/dataset.jl Outdated Show resolved Hide resolved
src/dataset.jl Outdated Show resolved Hide resolved
src/dataset.jl Show resolved Hide resolved
src/dataset.jl Outdated Show resolved Hide resolved
src/ogr/featuredefn.jl Outdated Show resolved Hide resolved
src/dataset.jl Outdated Show resolved Hide resolved
src/dataset.jl Show resolved Hide resolved
src/dataset.jl Outdated Show resolved Hide resolved
@maxfreu
Copy link
Contributor Author

maxfreu commented Aug 10, 2022

I just removed the writerasters again and implemented the comments you have made. I hope it looks better now :)

@maxfreu maxfreu marked this pull request as ready for review August 10, 2022 15:52
@maxfreu
Copy link
Contributor Author

maxfreu commented Aug 10, 2022

I just got the tests on windows to pass. The failing tests on mac look unrelated.

visr added a commit to JuliaGeo/Proj.jl that referenced this pull request Aug 10, 2022
@visr
test build
e58d60e 
yeesian/ArchGDAL.jl#315 (comment)

```
ERROR: LoadError: LoadError: InitError: could not load library "/Users/runner/.julia/artifacts/525a7050baaa2b0732225f4528525dfb3cfed1df/lib/libproj.25.dylib"
dlopen(/Users/runner/.julia/artifacts/525a7050baaa2b0732225f4528525dfb3cfed1df/lib/libproj.25.dylib, 1): Library not loaded: @rpath/libcurl.4.dylib
  Referenced from: /Users/runner/.julia/artifacts/525a7050baaa2b0732225f4528525dfb3cfed1df/lib/libproj.25.9.0.1.dylib
  Reason: Incompatible library version: libproj.25.dylib requires version 13.0.0 or later, but libcurl.4.dylib provides version 12.0.0
```
@visr visr mentioned this pull request Aug 10, 2022
Closed
yeesian
yeesian reviewed Aug 11, 2022
View reviewed changes
src/dataset.jl Outdated Show resolved Hide resolved
yeesian
yeesian reviewed Aug 11, 2022
View reviewed changes
src/dataset.jl Outdated Show resolved Hide resolved
@maxfreu
Copy link
Contributor Author

maxfreu commented Aug 11, 2022 •
edited
Loading

Ok next round. I improved the GPKG workaround, restored the old definition of deletefeaturedefn and changed GDTUnknown to Any in create. So I think the last point is whether to add the type annotation for dtype, which I think is maybe not necessary as the name already suggests what it is for.

yeesian
yeesian reviewed Aug 13, 2022
View reviewed changes
Copy link
Owner

@yeesian yeesian left a comment

Choose a reason for hiding this comment

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

Thank you so much, this is looking close to ready for merge!

I still have some reservations about the approach for GPKG but I might be convinced that this is a strict improvement over the current state of things and just need some time to develop confidence in the correctness of the GPKG workaround (via comparison with other compatibly-licensed GDAL packages).

src/dataset.jl Outdated Show resolved Hide resolved
yeesian
yeesian reviewed Aug 13, 2022
View reviewed changes
src/dataset.jl Outdated Show resolved Hide resolved
yeesian
yeesian reviewed Aug 13, 2022
View reviewed changes
src/dataset.jl Outdated Show resolved Hide resolved
yeesian
yeesian reviewed Aug 13, 2022
View reviewed changes
src/dataset.jl Outdated Show resolved Hide resolved
@maxfreu
Copy link
Contributor Author

maxfreu commented Aug 16, 2022

I just switched the default of use_gdal_copy to true, made per-layer options possible (given as vector instead of dict), changed the docs and added some tests.

yeesian
yeesian requested changes Aug 17, 2022
View reviewed changes
Copy link
Owner

@yeesian yeesian left a comment

Choose a reason for hiding this comment

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

LGTM once the remaining comments are addressed.

src/dataset.jl Outdated Show resolved Hide resolved
src/dataset.jl Outdated Show resolved Hide resolved
src/dataset.jl Outdated Show resolved Hide resolved
src/dataset.jl Outdated Show resolved Hide resolved
src/dataset.jl Outdated Show resolved Hide resolved
src/dataset.jl Outdated Show resolved Hide resolved
src/dataset.jl Outdated Show resolved Hide resolved
src/dataset.jl Outdated Show resolved Hide resolved
src/dataset.jl Outdated Show resolved Hide resolved
src/dataset.jl Outdated Show resolved Hide resolved
@maxfreu
Copy link
Contributor Author

maxfreu commented Aug 17, 2022

layer_options now expects a Dict and I changed some type annotations. Thanks for the comments! Hope it is fine now, but don't hesitate to insist if you still dislike sth.

yeesian
yeesian approved these changes Aug 18, 2022
View reviewed changes
Copy link
Owner

@yeesian yeesian left a comment

Choose a reason for hiding this comment

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

Thank you so much for pushing through on this!

@yeesian yeesian merged commit 04cd680 into yeesian:master Aug 18, 2022
@maxfreu
Copy link
Contributor Author

maxfreu commented Aug 18, 2022

Has been a pleasure! Thanks for the comments and let's see where this takes us :)

@visr visr mentioned this pull request Aug 18, 2022
Open
@yeesian yeesian mentioned this pull request Aug 19, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yeesian yeesian yeesian approved these changes

@visr visr visr left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@maxfreu @yeesian @visr