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

Analyze Terrain #2586

Merged
merged 3 commits into from
Jan 3, 2018
Merged

Analyze Terrain #2586

merged 3 commits into from
Jan 3, 2018

Conversation

rajadain
Copy link
Member

@rajadain rajadain commented Jan 2, 2018

Overview

Adds a Terrain tab to the Analyze stage. This uses the new RasterSummary operation in the geoprocessing service to get summary statistics for Elevation and Slope rasters.

Connects #2346

Demo

image

Also tagging @ajrobbins and @jfrankl for visual review.

Notes

  • There are no related visual layers corresponding to this tab currently

  • Can someone confirm the data source name? I took it from the mockup but want to make sure, and add it to the API doc.

  • At first I made the transposed table, which felt more natural in my mind:

    image

    Later I corrected it according to the mockup. The correction code is in the last two fixups, so can be easily reverted if we like this version better.

Testing Instructions

  • Check out this branch and reprovision worker to install the new geoprocessing service

    vagrant reload worker --provision
    
  • Go to :8000/ and select an area of interest. Proceed to Analyze.

  • Ensure you see a Terrain tab under Analyze. Ensure it has reasonable values. Ensure its design matches this mockup

This pulls in support for the RasterSummary operation introduced
in WikiWatershed/mmw-geoprocessing#79.

This will be used for Terrain analysis.
@ajrobbins
Copy link

I see the logic of the transposed table, but all of our other Analyze tables have the variable names & units on the top, so this should match. Let's keep it to matching the mockup.

The data source name should be: "NHDPlus V2 NEDSnapshot DEM"

@kellyi
Copy link
Contributor

kellyi commented Jan 3, 2018

Setting this up to review.

@kellyi
Copy link
Contributor

kellyi commented Jan 3, 2018

Are we adjusting the size of the panel as part of this work or will we do that subsequently? As pictured above the Terrain tab currently goes onto a second row and when "Point Sources" or "Water Quality" is highlighted, both "Terrain" and "Streams" are sent to the second row.

@rajadain
Copy link
Member Author

rajadain commented Jan 3, 2018

Are we adjusting the size of the panel as part of this work or will we do that subsequently?

I think subsequently. @jfrankl is tagged for visual review here, and if he has any immediate suggestions I'll implement, but I've also heard talk that we may be dropping some analyses, which will make room.

@kellyi
Copy link
Contributor

kellyi commented Jan 3, 2018

Working well! Large AOI timed out on my local but smaller AOI worked fine:

screen shot 2018-01-03 at 2 53 09 pm

screen shot 2018-01-03 at 2 53 20 pm

Starts a job to produce summary statistics for elevation and slope in a
given area.

TODO Where does the NED and Slope data come from?
Copy link
Contributor

Choose a reason for hiding this comment

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

cc @arottersman -- do you know the answer to this?

Choose a reason for hiding this comment

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

NED data comes from NHDPlus V2 NEDSnapshot, and Slope data is from NED (National Elevation Dataset)

categories = [
dict(type='average',
elevation=(result[0]['avg'] * M_PER_CM),
slope=result[1]['avg']),
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably fine since we're just unpacking the results but if we wanted to assign these beforehand I think we could do:

[elevation, slope] = result
categories = [
...

and then use elevation and slope in place of result[0] & result[1]. Arguable whether that's an improvement, though.

Copy link
Member Author

Choose a reason for hiding this comment

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

I like it! Will fix.

@log_request
def start_analyze_terrain(request, format=None):
"""

Copy link
Contributor

Choose a reason for hiding this comment

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

Should this have a docstring?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch meant to put that in.

Copy link
Contributor

@kellyi kellyi left a comment

Choose a reason for hiding this comment

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

+1. LGTM.

May be worth figuring out the proper text for those docstrings before merging since we may not come back just to fix those if they aren't set here.

@rajadain
Copy link
Member Author

rajadain commented Jan 3, 2018

Fixed source name, docstrings, and code style. Going to squash and merge shortly.

Including an endpoint, Celery task, and Geoprocessing template.
Including table templates, analyze result view, census model,
and add to set of MMW Analyze tasks.
@rajadain
Copy link
Member Author

rajadain commented Jan 3, 2018

Screenshot with updated source:

image

Merging this now. Thanks for reviewing!

@rajadain rajadain merged commit 5bade6d into develop Jan 3, 2018
@rajadain rajadain deleted the tt/analyze-terrain branch January 3, 2018 20:35
@kellyi kellyi assigned rajadain and unassigned kellyi Jan 4, 2018
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.

None yet

4 participants