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

Streamline GeoJSON import #346 #412

Merged
merged 6 commits into from
Mar 31, 2023
Merged

Streamline GeoJSON import #346 #412

merged 6 commits into from
Mar 31, 2023

Conversation

m-mohr
Copy link
Member

@m-mohr m-mohr commented Mar 8, 2023

Streamlines the GeoJSON import #346

The load_geojson function probably needs a bit better description on how to construct the data cube.

@m-mohr m-mohr added the vector label Mar 8, 2023
@m-mohr m-mohr added this to the 2.0.0 milestone Mar 8, 2023
@m-mohr m-mohr linked an issue Mar 8, 2023 that may be closed by this pull request
@m-mohr m-mohr marked this pull request as ready for review March 8, 2023 11:45
@m-mohr m-mohr mentioned this pull request Mar 8, 2023
@clausmichele
Copy link
Member

If I understand correctly, after that this PR will be merged, when a user will select a polygon aoi in a load_collection process in the Web Editor there will be two processes added: load_geojson and load_collection ?

@m-mohr
Copy link
Member Author

m-mohr commented Mar 8, 2023

In the Web Editor it would complicate things a bit as you'd need to add the load_geojson specifically to the model builder and then connect it to the temporal_extent parameter, but on the other hand it simplifies, because you can reuse it in subsequent processes like aggregate_spatial.

@soxofaan
Copy link
Member

soxofaan commented Mar 8, 2023

I just had a quick look at this PR, but this looks like a heavily breaking change.
Can't we just first add the load_geojson option in non-breaking way (keep inline geosjon option),
and then in a later phase remove the original inline geojson option?

@m-mohr
Copy link
Member Author

m-mohr commented Mar 8, 2023

You can always still support geojson by adding the missing schema again. Having it still in here in 2.0 as deprecated would also mean new implementations would face it, which is not ideal.

@m-mohr m-mohr mentioned this pull request Mar 10, 2023
@m-mohr
Copy link
Member Author

m-mohr commented Mar 10, 2023

  • Deprecate in stable processes
  • Don't add to new processes
  • Remove from proposals

8+

@soxofaan soxofaan mentioned this pull request Mar 10, 2023
@m-mohr m-mohr linked an issue Mar 10, 2023 that may be closed by this pull request
@m-mohr
Copy link
Member Author

m-mohr commented Mar 10, 2023

The PR has been updated according to the discussions today. Please review.

@m-mohr m-mohr linked an issue Mar 10, 2023 that may be closed by this pull request
# Conflicts:
#	proposals/filter_vector.json
#	proposals/vector_buffer.json
#	proposals/vector_to_random_points.json
#	proposals/vector_to_regular_points.json
@m-mohr m-mohr added help wanted Extra attention is needed discussion needed and removed help wanted Extra attention is needed labels Mar 14, 2023
@m-mohr m-mohr self-assigned this Mar 29, 2023
# Conflicts:
#	CHANGELOG.md
#	load_collection.json
#	proposals/filter_vector.json
#	proposals/load_ml_model.json
#	proposals/load_result.json
@m-mohr
Copy link
Member Author

m-mohr commented Mar 31, 2023

Applied the deprecation as discussed. Will create a new PR for load_geojson.

@m-mohr m-mohr merged commit e251d05 into draft Mar 31, 2023
@m-mohr m-mohr deleted the geojson branch March 31, 2023 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants