-
Notifications
You must be signed in to change notification settings - Fork 31
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 Drainage Area Draw Tools #3551
Conversation
Currently the implementation copies Square Km Stamp for Point Selection and Draw Tool for Stream Selection, but it lays the visual groundwork for the next phase.
Since drainage areas will need API interaction to draw correctly, we add a new family of APIs nested under the draw/ path. This dummy implementation returns a buffered shape around the given point, but will eventually be replaced with an API call to the Drexel / ANS endpoint. The front-end is updated to call this API when using the point- based drainage area draw tool. This is currently a naive POST, but may be converted to a taskRunner based implementation, a-la RWD, if the Drexel / ANS endpoint ends up taking a while to respond.
Similar to the point one, this also sends a request to the API, and shows the returned shape. When this draw tool is activated, the High Resolution Streams layer is turned on, so that the user can draw an area around a stream. This implementation currently does not have any error handling, or support for delays. It is likely that we'll need to transition these endpoints to a Celery task based workflow, as is the case for all the other Geoprocessing API endpoints.
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.
The new drainage area draw tools work well in general. Good job on this black box!
I noticed the app will return errors like this in these two scenarios:
- using point-based tool when not signed in ->
403
- using stream based tool before the stream layer is fully loaded, especially when zoomed out a bit -> polygon payload to endpoint causing
500
.
01ae5a6 should cover the first point above. I'll take a look at the second one. The streams being displayed are just for the user's benefit, the intersection happens on the server side and should be unrelated. But perhaps when zoomed out the stream intersection is too big. |
I was going to defer fixing the error box until later, but will see if I can easily capture it in this PR as well. |
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.
All works well! I agree with deferring the error box fix and focus this PR in the drainage draw tool
Previously the endpoints were made as simple request-response functions. However, given that calls to the third party API may take a while, it is better to make them task-based, as all other public APIs are, and return the results via the job endpoint. The front-end is updated correspondingly to show a spinner and error messages as appropriate while waiting for the task to finish.
For clarity, show the point or stream segment selected by the user on screen along with the area of interst. We reuse the same setup as for RWD, so if the area of interest is changed the additional layers will be removed as well.
Thanks for reviewing! Would really appreciate you evaluating the following additional functionality. The two additional commits above add important features:
DemoTesting Instructions
|
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 tested the point-based and the stream-based tool - both works well! Nice job in refactoring it to a task. The points and segments are helpful for users.
src/mmw/js/src/draw/views.js
Outdated
polling: false, | ||
}); | ||
console.error(response.error); | ||
deferred.reject('Drainage area could not be calculated'); |
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.
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.
Thanks for prompting this. Fixed in eb2c372:
Thanks for the detailed review and continued encouragement for better user experience! |
Overview
Adds two new draw tools for Drainage Area, with dummy implementations. The first takes a point, and requests an area that drains to it. The second takes a polygon, drawn around streams, and requests and area that drains to those streams. The dummy implementation simply buffers the point or stream by 0.1 and returns the shape.
In upcoming changes, the source points / streams will be sent to the Drexel / ANS API, and we'll get the correct shapes from them.
The returned shape also has
drainage_area: true
property, which will be used to show special drainage area analysis.Connects #3546
Demo
Notes
In the current implementation the new draw endpoints are synchronous. However, we may need to make them Celery task based, as are all the other analyze and model endpoints, since the Drexel API may take a while to run.
There is also more validation that needs to be done, and error handling added, so that validation errors are communicated clearly to the user. For example, if in your stream selection shape, there are no streams, there is currently no graceful error handling for that.
Testing Instructions
./scripts/bundle.sh --debug