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

Add DerivedRegions from Region Union/Intersection #19

Merged
merged 44 commits into from
Oct 4, 2023

Conversation

jjnesbitt
Copy link
Member

@jjnesbitt jjnesbitt commented Sep 12, 2023

Closes #14

This PR adds derived region creation functionality, currently only allowing individual region union as a way to create these derived regions. More functionality will be implemented as part of #25.

This PR is rather bulky, as it involves a large re-work to the web app. Ideally this rework wouldn't be bundled into this PR, but it was done in the pursuit of supporting Derived Regions. The plan is to merge this PR with as little additional changes as possible, now that the bulk of the re-work is done, and derived regions are supported in some fashion. Further iteration can be done in follow up PRs.

@jjnesbitt jjnesbitt force-pushed the region-operations branch 7 times, most recently from a1a3470 to f7e17fb Compare September 19, 2023 14:44
@jjnesbitt
Copy link
Member Author

@annehaley lmk what you think of c48241a (#19). This is the rework of that function we talked about.

@annehaley
Copy link
Collaborator

@annehaley lmk what you think of c48241a (#19). This is the rework of that function we talked about.

Yeah, I like that organization a lot more. However I do notice that the createVectorLayer function is only used for derived regions, and every Dataset with geodata_file defined uses createVectorTileLayer. There should be a separation between vector datasets that use tiles and vector datasets that don't. So if a Dataset has vector_tiles_file populated, it would use the tiled source. If it doesn't have a vector_tiles_file but does have a geodata_file, then a normal vector source should be used.

Unless, of course, these changes depend on your vector tile optimization. In that case, would every vector dataset use tiling?

@jjnesbitt jjnesbitt mentioned this pull request Sep 21, 2023
3 tasks
@jjnesbitt
Copy link
Member Author

@annehaley lmk what you think of c48241a (#19). This is the rework of that function we talked about.

Yeah, I like that organization a lot more. However I do notice that the createVectorLayer function is only used for derived regions, and every Dataset with geodata_file defined uses createVectorTileLayer. There should be a separation between vector datasets that use tiles and vector datasets that don't. So if a Dataset has vector_tiles_file populated, it would use the tiled source. If it doesn't have a vector_tiles_file but does have a geodata_file, then a normal vector source should be used.

Unless, of course, these changes depend on your vector tile optimization. In that case, would every vector dataset use tiling?

I see what you mean. I do think they should be handled differently, but I'm confused on how it would work in this scenario. For a dataset that does not have vector_tiles_file defined, but does have geodata_file defined, and isn't a region dataset, what source would be used for the vector layer? Currently, the only way a vector layer is created from a dataset is through regions.

If we don't have a clear answer to the above question, I think it might be okay to just wait until the vector tiles optimization is merged in. In that case, derived regions may be the only "user" of vector layers, since we could just use vector tiles for everything else. We could even use vector tiles for derived regions as well, but that might be overkill.

What do you think?

@jjnesbitt jjnesbitt marked this pull request as ready for review September 21, 2023 18:10
@jjnesbitt jjnesbitt requested a review from annehaley September 21, 2023 18:10
@jjnesbitt
Copy link
Member Author

jjnesbitt commented Sep 21, 2023

Also, I'll work to resolve the current conflicts since #20 is now merged.

@annehaley
Copy link
Collaborator

I see what you mean. I do think they should be handled differently, but I'm confused on how it would work in this scenario. For a dataset that does not have vector_tiles_file defined, but does have geodata_file defined, and isn't a region dataset, what source would be used for the vector layer? Currently, the only way a vector layer is created from a dataset is through regions.

If we don't have a clear answer to the above question, I think it might be okay to just wait until the vector tiles optimization is merged in. In that case, derived regions may be the only "user" of vector layers, since we could just use vector tiles for everything else. We could even use vector tiles for derived regions as well, but that might be overkill.

What do you think?

For Datasets, vector_tiles_file should be used as the input for a VectorTileSource if it exists. Else if geodata_file exists, it should be used as the input for a VectorSource. The contents of geodata_file are a geojson that can be used as vector data.

I definitely want to get your tiling optimization involved, but this doesn't need to depend on it. I still think the size of the geojson data should be our threshold for determining whether vector data should be tiled (whether that be a vector Dataset, Region, or DerivedRegion, though the latter two are not likely to exceed a size threshold). That way we don't end up creating too many VectorTile objects for vector data that doesn't need to be tiled.

@jjnesbitt jjnesbitt force-pushed the region-operations branch 2 times, most recently from 067da3b to 58d9481 Compare September 25, 2023 16:49
@jjnesbitt
Copy link
Member Author

For Datasets, vector_tiles_file should be used as the input for a VectorTileSource if it exists. Else if geodata_file exists, it should be used as the input for a VectorSource. The contents of geodata_file are a geojson that can be used as vector data.

This has been addressed in 58d9481

@jjnesbitt
Copy link
Member Author

I still think the size of the geojson data should be our threshold for determining whether vector data should be tiled (whether that be a vector Dataset, Region, or DerivedRegion, though the latter two are not likely to exceed a size threshold). That way we don't end up creating too many VectorTile objects for vector data that doesn't need to be tiled.

I think that makes sense, although I'll point out that I think the current threshold for this of 100mb is far too high, as I see lag when rendering datasets even in the single MB range of sizes. So realistically the range of sizes that we can afford not to tile with vector tiles may end up being small, but nonetheless I agree.

Regarding creating too many VectorTile objects, I don't think that's a real concern right now. We would have to expand much beyond the scope of the current project to create enough of these tiles to see performance issues, and even if it came to that, indexing / optimization should get us very far in that regard.

Copy link
Collaborator

@annehaley annehaley left a comment

Choose a reason for hiding this comment

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

This is just a first pass at a review; I haven't checked out the branch to try it yet. But These are just some questions and organization suggestions I had from just looking at the diff.

uvdat/core/tasks/conversion.py Show resolved Hide resolved
uvdat/core/views.py Outdated Show resolved Hide resolved
uvdat/core/views.py Outdated Show resolved Hide resolved
uvdat/core/views.py Show resolved Hide resolved
web/src/components/MainDrawerContents.vue Show resolved Hide resolved
web/src/components/OpenLayersMap.vue Outdated Show resolved Hide resolved
@jjnesbitt jjnesbitt force-pushed the region-operations branch 2 times, most recently from 4ad5916 to 7764baa Compare September 28, 2023 15:14
@annehaley
Copy link
Collaborator

annehaley commented Oct 3, 2023

@AlmightyYakob I tried out the new Active Layers overlay and thought it could use a couple things:

  • Tooltips on the icons so the user can understand their functions better (especially important for a button that clears all layers)
  • Text to fill the component when there are no active layers to show
  • Some spacing adjustments

I made these changes in 2e0b8d4, let me know what you think.
Otherwise, I really like how this component looks, and I think it is a better place for this information.

Copy link
Collaborator

@annehaley annehaley 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 for accommodating my suggestions from the previous review :)
The only things I thought of on this pass were minor. I made a few commits for small things, let me know what you think.

web/src/data.ts Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

The content of this file is good, just wondering if we can give it a more specific name / location? Maybe web/src/map/utils.ts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this file can go in your new map directory, rather than at the src level.

Copy link
Member Author

Choose a reason for hiding this comment

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

Both this and the data.ts file are used in other components outside of the map directory. Would you be okay leaving this as-is for now, and fixing it through #27?

@jjnesbitt
Copy link
Member Author

@AlmightyYakob I tried out the new Active Layers overlay and thought it could use a couple things:

  • Tooltips on the icons so the user can understand their functions better (especially important for a button that clears all layers)
  • Text to fill the component when there are no active layers to show
  • Some spacing adjustments

I made these changes in 2e0b8d4, let me know what you think. Otherwise, I really like how this component looks, and I think it is a better place for this information.

Just checked it out, looks great! Thanks for those changes.

@jjnesbitt jjnesbitt merged commit 948f6ad into master Oct 4, 2023
@jjnesbitt jjnesbitt deleted the region-operations branch October 4, 2023 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support more complex region operations
2 participants