Skip to content

/ arcgis-python-api

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

lidar guides parts 1 and 2 #610

Open
wants to merge 2 commits into
base: master
from
Open

lidar guides parts 1 and 2 #610

wants to merge 2 commits into from

Conversation

@CMPeng
Copy link
Collaborator

CMPeng commented Feb 21, 2020

These files belong into the series of LiDAR guides, with a sequence of -

  • Part 1 - lidar_basics_part1 (basic concepts)
    - lidar_basics_web_scene_part1a (visualizing LiDAR in scene viewer)
    - lidar_basics_operations_part1b (creating lidar dataset in arcpy, etc.)
  • Part 2 - lidar_image_processing_part2 (DSM, DTM, CHM and TIN with arcpy)
    - lidar_create_hillshade_using_gdal_part2b (DSM, DTM, CHM creation and visualization in GDAL)
  • Part 3 - lidar_pdal_lib_part3 (using PDAL, including PDAL's online browsing tool)
  • Part 4 - lidar_laspy_lib_part4 (using laspy, including 3d reconstruction)
  • Part 5 - lidar_extract_bldgs_classified_part5 (arcpy, extract buildings with classification codes)
  • Part 6 - lidar_extract_bldgs_unclassfied_part6 (arcpy, extract buildings without classification codes)

Please find in these PR the notebooks for Part 1 and Part 2. Thanks!
@CMPeng
lidar guides parts 1 and 2
Loading status checks…
9ddc9d4
@review-notebook-app
Copy link

review-notebook-app bot commented Feb 21, 2020

Check out this pull request on  ReviewNB

You'll be able to see Jupyter notebook diff and discuss changes. Powered by ReviewNB.
@CMPeng CMPeng requested review from AtmaMani, jyaistMap and DavidJVitale Feb 21, 2020
@CMPeng
fix missing images
Loading status checks…
24f5f15
@CMPeng CMPeng mentioned this pull request Apr 3, 2020
Open
@AtmaMani
AtmaMani reviewed Apr 7, 2020
View changes
guide/06-imagery-and-raster-analysis/lidar_basics_part1.ipynb
@@ -0,0 +1,215 @@
{

This comment has been minimized.

Sign in to view
@AtmaMani

AtmaMani Apr 7, 2020

Member

Change to "Working with LiDAR data" -> Heading level 1

"Part 1 - Introductory concepts" -> Heading level 2


Let us will keep the level1 heading same for all notebooks in this series and change the level 2 for each notebook. Let us align the filename of the notebook with the level 2 heading.


Reply via ReviewNB

guide/06-imagery-and-raster-analysis/lidar_basics_part1.ipynb
@@ -0,0 +1,215 @@
{

This comment has been minimized.

Sign in to view
@AtmaMani

AtmaMani Apr 7, 2020

Member
  • 2nd paragraph: These receivers .... the range (distance) ... (put distance within parenthesis)


  • First return: ...user often gets... -> users often gets exposed.
  • One emitted laser pulse -> A single emitted laser...
  • that encounters multiple reflection surfaces -> that encounters multiple reflective surfaces...



Reply via ReviewNB

guide/06-imagery-and-raster-analysis/lidar_basics_part1.ipynb
@@ -0,0 +1,215 @@
{

This comment has been minimized.

Sign in to view
@AtmaMani

AtmaMani Apr 7, 2020

Member

"The list below includes common examples of...." -> Below is a list of different things you can do with LAS Datasets

"Quickly display LiDAR data as point clouds or as a triangulated surface in 2D and 3D." (added the word 'as')


Reply via ReviewNB

guide/06-imagery-and-raster-analysis/lidar_basics_part1.ipynb
@@ -0,0 +1,215 @@
{

This comment has been minimized.

Sign in to view
@AtmaMani

AtmaMani Apr 7, 2020

Member

We may have to edit the names and hyperlinks to other parts after we rename the rest of the files.


Reply via ReviewNB

@AtmaMani
Copy link
Member

AtmaMani commented Apr 7, 2020

@CMPeng I have posted comments for Notebook 1 above. I am in the process of reviewing the rest of the files.
Meanwhile, can you make the following changes
Rename files:
I prefer the naming convention used in NA module docs. So, can you rename

  • lidar_basics_part1.ipynb -> part1_lidar_basics.ipynb
  • lidar_basics_web_scene_part1a.ipynb -> part2_lidar_visualization.ipynb
  • lidar_basics_operations_part1b.ipynb -> part3_lidar_management_and_editing.ipynb
  • lidar_image_processing_part2.ipynb -> part4_lidar_producing_elevation_models.ipynb

For part2b and the rest of the notebooks, I can suggest their names after I review the contents in detail.
@review-notebook-app
Copy link

review-notebook-app bot commented Apr 7, 2020

View / edit / reply to this conversation on ReviewNB

AtmaMani commented on 2020-04-07T23:00:07Z
----------------------------------------------------------------

Change to "Working with LiDAR data" -> Heading level 1

"Part 3 - Data management and editing" -> Heading level 2
@review-notebook-app
Copy link

review-notebook-app bot commented Apr 7, 2020

View / edit / reply to this conversation on ReviewNB

AtmaMani commented on 2020-04-07T23:00:08Z
----------------------------------------------------------------

Change to "Working with LiDAR data" -> Heading level 1

"Part 2 - Visualizing point clouds in scene viewer" -> Heading level 2
@review-notebook-app
Copy link

review-notebook-app bot commented Apr 7, 2020

View / edit / reply to this conversation on ReviewNB

AtmaMani commented on 2020-04-07T23:00:10Z
----------------------------------------------------------------

Change to "Working with LiDAR data" -> Heading level 1

"Part 4 - Producing elevation models" -> Heading level 2
@review-notebook-app
Copy link

review-notebook-app bot commented Apr 10, 2020 •
edited

View / edit / reply to this conversation on ReviewNB

AtmaMani commented on 2020-04-10T00:32:06Z
----------------------------------------------------------------

Here, we need to explain how users can publish their LAS datasets, something like:


In order to visualize the LAS datasets with Python API, we need to be published as a special type of web layer called 'Point Cloud Scene Layer'. To learn more about this, refer to publishing point cloud scene layers using ArcGIS Pro

AtmaMani commented on 2020-04-10T04:25:07Z
----------------------------------------------------------------

When reviewing part 3 (current part 1b) I noticed publishing is covered there. So we could instead mention that publishing is demonstrated in part 3 of the LiDAR guide series.
@review-notebook-app
Copy link

review-notebook-app bot commented Apr 10, 2020

View / edit / reply to this conversation on ReviewNB

AtmaMani commented on 2020-04-10T00:32:08Z
----------------------------------------------------------------

Would be good to hyperlink the plenary video if available publicly on YouTube
@review-notebook-app
Copy link

review-notebook-app bot commented Apr 10, 2020

View / edit / reply to this conversation on ReviewNB

AtmaMani commented on 2020-04-10T00:32:08Z
----------------------------------------------------------------

Change heading to "Stop 1: Railway"
@review-notebook-app
Copy link

review-notebook-app bot commented Apr 10, 2020

View / edit / reply to this conversation on ReviewNB

AtmaMani commented on 2020-04-10T00:32:09Z
----------------------------------------------------------------

"Stop 2: Regional Center" Lets also change the rest of the headings similarly
@review-notebook-app
Copy link

review-notebook-app bot commented Apr 10, 2020

View / edit / reply to this conversation on ReviewNB

AtmaMani commented on 2020-04-10T00:32:10Z
----------------------------------------------------------------

Change to "Comparatively, the same scene being viewed at Google Maps is like" -


@review-notebook-app
Copy link

review-notebook-app bot commented Apr 10, 2020

View / edit / reply to this conversation on ReviewNB

AtmaMani commented on 2020-04-10T00:32:10Z
----------------------------------------------------------------

"The equivalent scene when viewed from Google Maps is like - "
@review-notebook-app
Copy link

review-notebook-app bot commented Apr 10, 2020

View / edit / reply to this conversation on ReviewNB

AtmaMani commented on 2020-04-10T00:32:11Z
----------------------------------------------------------------

PR https://github.com/ArcGIS/geosaurus/pull/3436 adds sync nav to the Python API. Can we rewrite this part of notebook to use the built-in capability?
@review-notebook-app
Copy link

review-notebook-app bot commented Apr 10, 2020

View / edit / reply to this conversation on ReviewNB

AtmaMani commented on 2020-04-10T00:32:12Z
----------------------------------------------------------------

Since this location is not in photorealistic 3D on Google Maps, can we rather create a map widget, set basemap to satellite and display it?
@review-notebook-app
Copy link

review-notebook-app bot commented Apr 10, 2020

View / edit / reply to this conversation on ReviewNB

AtmaMani commented on 2020-04-10T00:32:13Z
----------------------------------------------------------------

Title this paragraph as 'Conclusion'.


We can add a point about how point clouds appear in comparison to photorealistic 3D. So I suggest changing the paragraph 1 to:


In this notebook, we have explored how the LiDAR point clouds can be viewed in Scene Viewer, how LiDAR point clouds appear when visualized and compared it to photorealistic 3D renderings of the same site. We also observed, how configuring the scene properties such as center, extent, tilt, heading and zoom can shift from one site to another, how to sync navigation between different map widgets and how to study various aspects of the scene by changing the drawing styles of the LiDAR point clouds.

@review-notebook-app
Copy link

review-notebook-app bot commented Apr 10, 2020

View / edit / reply to this conversation on ReviewNB

AtmaMani commented on 2020-04-10T23:50:42Z
----------------------------------------------------------------

It would be good to add a note here saying arcpy is needed to run this notebook. Users can run this with ArcGIS Notebooks in ArcGIS Enterprise / Online / Pro
@review-notebook-app
Copy link

review-notebook-app bot commented Apr 10, 2020

View / edit / reply to this conversation on ReviewNB

AtmaMani commented on 2020-04-10T23:50:43Z
----------------------------------------------------------------

...will not provide users consistently, reproducible indication... (remove the comma in this sentence)
@review-notebook-app
Copy link

review-notebook-app bot commented Apr 10, 2020

View / edit / reply to this conversation on ReviewNB

AtmaMani commented on 2020-04-10T23:50:44Z
----------------------------------------------------------------

...is to be used .... -> can be used...

To find out more... (missing word 'to')
@review-notebook-app
Copy link

review-notebook-app bot commented Apr 10, 2020

View / edit / reply to this conversation on ReviewNB

AtmaMani commented on 2020-04-10T23:50:44Z
----------------------------------------------------------------

We can rewrite this snippet using the synced navigation feature now available in the API. Simply create the 2nd map and link it to first and the 2nd will inherit the extent, and attitude info (heading, tilt, etc)
@review-notebook-app
Copy link

review-notebook-app bot commented Apr 10, 2020

View / edit / reply to this conversation on ReviewNB

AtmaMani commented on 2020-04-10T23:50:45Z
----------------------------------------------------------------

A good place to start is four times the average point spacing. (missing word spacing at the end of the sentence).
@review-notebook-app
Copy link

review-notebook-app bot commented Apr 10, 2020

View / edit / reply to this conversation on ReviewNB

AtmaMani commented on 2020-04-10T23:50:46Z
----------------------------------------------------------------

Can we use the new arcgis.raster.Raster class for this (instead of arcpy.Raster)?
@review-notebook-app
Copy link

review-notebook-app bot commented Apr 10, 2020

View / edit / reply to this conversation on ReviewNB

AtmaMani commented on 2020-04-10T23:50:46Z
----------------------------------------------------------------

It is not clear why you access an Imagery Layer / service. Now that the arcgis.Raster object can render local rasters through ArcPy, can you simply access the local raster show a preview here?
@review-notebook-app
Copy link

review-notebook-app bot commented Apr 10, 2020

View / edit / reply to this conversation on ReviewNB

AtmaMani commented on 2020-04-10T23:50:47Z
----------------------------------------------------------------

Same here, can you access the local raster and display?
@review-notebook-app
Copy link

review-notebook-app bot commented Apr 10, 2020

View / edit / reply to this conversation on ReviewNB

AtmaMani commented on 2020-04-10T23:50:48Z
----------------------------------------------------------------

Can you try to get histograms from arcgis.raster.Raster object and rewrite this using local raster data?
@review-notebook-app
Copy link

review-notebook-app bot commented Apr 10, 2020

View / edit / reply to this conversation on ReviewNB

AtmaMani commented on 2020-04-10T23:50:48Z
----------------------------------------------------------------

Can we heading "Step 1:..." as "Step 3.3.1..." and similar for rest of the headings under section 3.3
@review-notebook-app
Copy link

review-notebook-app bot commented Apr 10, 2020

View / edit / reply to this conversation on ReviewNB

AtmaMani commented on 2020-04-10T23:50:49Z
----------------------------------------------------------------

can you make the arguments as name=value for this snippet?
@review-notebook-app
Copy link

review-notebook-app bot commented Apr 10, 2020

View / edit / reply to this conversation on ReviewNB

AtmaMani commented on 2020-04-10T23:50:50Z
----------------------------------------------------------------

Part 3 is good for the most part. The pieces of code that used Imagery Layer and then switched to arcpy code and local layer are a bit confusing. You are probably doing it so the outputs can be visualized. Now that the API supports local raster viz and the new arcgis.raster.Raster class for local rasters, I recommend rewriting some of these snippets where appropriate
@review-notebook-app
Copy link

review-notebook-app bot commented Apr 27, 2020

View / edit / reply to this conversation on ReviewNB

AtmaMani commented on 2020-04-27T16:51:29Z
----------------------------------------------------------------

I like the content, well chosen. Can we use the code formatting to refer to variables / code fragments / names of library methods / classes? For instance, instead of showing hydro enforcement as with code formatting, we can make it bold. This will keep it consistent with the rest of the SDK docs.
@review-notebook-app
Copy link

review-notebook-app bot commented Apr 27, 2020

View / edit / reply to this conversation on ReviewNB

AtmaMani commented on 2020-04-27T16:51:30Z
----------------------------------------------------------------

Are we using ArcMap tutorial dataset? Let us discuss about how this data can be made available.


Can we make root_folder a variable? The user can modify it early on and rest of the notebook would work.
@review-notebook-app
Copy link

review-notebook-app bot commented Apr 27, 2020

View / edit / reply to this conversation on ReviewNB

AtmaMani commented on 2020-04-27T16:51:31Z
----------------------------------------------------------------

Casing typo in "ArcGIs Online"
@AtmaMani
AtmaMani reviewed Apr 27, 2020
View changes
guide/06-imagery-and-raster-analysis/lidar_create_hillshade_using_gdal_part2b.ipynb
@@ -0,0 +1,642 @@
{

This comment has been minimized.

Sign in to view
@AtmaMani

AtmaMani Apr 27, 2020

Member

Is this the data used in earlier Part 4 as well? If yes, can we move this to an earlier part of the series?


I also think we will check this Part 5 into the Geosaurus repo instead of this repo for now.


Reply via ReviewNB

@AtmaMani
Copy link
Member

AtmaMani commented Apr 27, 2020 •
edited

@CMPeng I have finished reviewing all parts of this PR and have posted my comments.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AtmaMani AtmaMani

@jyaistMap jyaistMap

@DavidJVitale DavidJVitale

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
@CMPeng @AtmaMani
You can’t perform that action at this time.