-
Notifications
You must be signed in to change notification settings - Fork 56
Add add_vector_layer function #445
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
base: main
Are you sure you want to change the base?
Conversation
for more information, see https://pre-commit.ci
Integration tests report: appsharing.space |
**kwargs, | ||
) | ||
else: | ||
df = gpd.read_file( |
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.
I guess this will download the GeoJSON if I do something like:
url = "https://github.com/opengeos/datasets/releases/download/world/countries.geojson"
doc.add_vector_layer(path=url, name="GeoJSON")
This is unfortunate since it will embed the GeoJSON data into the JGIS file, which may be an unwanted behavior. Keeping the GeoJSON source pointed by URL may be better.
So I guess we'd want to consider two approaches in this function:
- if it's a URL, we keep the URL as-is and don't embed the data
- if it's a local file or some in-memory data in Python, we don't have the choice but to embed the data so we keep this logic here
@martinRenou @giswqs how can we get this PR moving forward? On Martin's comment above, I prefer not embedding the data in the project file. GeoJSON files can get quite large, and I think if we're ever going to be embedding data in the project file we should consider a string-encoded binary format. |
There are cases we embed JSON data already (e.g. when passing data from Python's memory directly, instead of through file url/path), we should definitely improve things and consider another approach 👍🏽 |
Yeah. I think it makes sense to punt on a better format until later :) |
We're looking to get this PR over the finish line. We need to rebase to start with. Then, we discussed a plan which we're not sure how to implement, and I'd like to revisit the minimal version of this that we can complete tomorrow.
I think we've bitten off a big task in this plan and that we can break it into two concerns:
|
Thanks all for looking into this. I agree with most of the things mentioned. Rebase shall be easy since no conflicts.
I like that we would not be embedding the geojson in
Infact, I don't think our typescript logic supports something like this yet. In theory, if we want to do this - we'd have to store the path of both Cached Source & Actual Source in the schema & do the thing you're suggesting.
That's the core blocker - we simply can't use
Agreed - this split makes sense. Thanks again, everyone. I hope each of you gets to contribute your piece, and that you’re enjoying the hack! |
Fix #437
This PR adds:
add_vector_layer
examples.📚 Documentation preview: https://jupytergis--445.org.readthedocs.build/en/445/
💡 JupyterLite preview: https://jupytergis--445.org.readthedocs.build/en/445/lite