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

Pdal opals notebook #67

Closed
wants to merge 24 commits into from
Closed

Pdal opals notebook #67

wants to merge 24 commits into from

Conversation

GwydionJon
Copy link
Collaborator

@GwydionJon GwydionJon commented Oct 5, 2021

Fixed opals to pdal conversion by introducing a class variable spatial_ref. When creating a pdal filter this is automatically set, only when creating a opals filter an extra step is taken and the spatial reference data is extracted with a pdal reader.

@codecov
Copy link

codecov bot commented Oct 7, 2021

Codecov Report

Merging #67 (37e271b) into main (518143a) will increase coverage by 0.11%.
The diff coverage is 81.08%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #67      +/-   ##
==========================================
+ Coverage   85.59%   85.71%   +0.11%     
==========================================
  Files          13       13              
  Lines        1090     1120      +30     
==========================================
+ Hits          933      960      +27     
- Misses        157      160       +3     
Impacted Files Coverage Δ
adaptivefiltering/pdal.py 91.97% <75.00%> (-3.71%) ⬇️
adaptivefiltering/dataset.py 90.90% <100.00%> (+0.58%) ⬆️
adaptivefiltering/opals.py 95.30% <100.00%> (+0.09%) ⬆️
adaptivefiltering/filter.py 95.71% <0.00%> (+2.85%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 518143a...37e271b. Read the comment docs.

@dokempf
Copy link
Member

dokempf commented Oct 8, 2021

You somehow removed the LFS pointers and added the data files as non-LFS content. That would need to be reverted...

jupyter/demo.ipynb Outdated Show resolved Hide resolved
Copy link
Member

@dokempf dokempf left a comment

Choose a reason for hiding this comment

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

I made some comments, mostly minor. However, there is one major showstopper for tomorrow's meeting: I looked at the visualization output and I am not sure we are seeing the expected result (results too similar compared to Bernhard's presentation). I need more time to investigate that, so I would postpone the merge.

@@ -144,3 +144,4 @@ dmypy.json

# Cython debug symbols
cython_debug/
adaptivefiltering/data/uls_thingstaette.las
Copy link
Member

Choose a reason for hiding this comment

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

Should be removed, this is only your local setup of where to place the file.

"source": [
"# Opals Example Notebook\n",
"This notebook is a short example of the Opals utilities provided by `adaptivefiltering`.\n",
"If you have not worked with `adaptivefiltering` before please have a look at the `demo.ipynb`.\n",
Copy link
Member

Choose a reason for hiding this comment

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

demo.ipynb does not exist anymore

"outputs": [],
"source": [
"dataset = adaptivefiltering.DataSet(filename=\"data/uls_thingstaette.las\")\n",
"adaptivefiltering.opals.set_opals_directory(\"/home/gwydion/SSC/Opals/opals_2.4.0\")"
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this line.

"metadata": {},
"outputs": [],
"source": [
"dataset = adaptivefiltering.DataSet(filename=\"data/uls_thingstaette.las\")\n",
Copy link
Member

Choose a reason for hiding this comment

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

This should not have the data/ prefix as we will not ship it in the data directory of adaptivefiltering.

"metadata": {},
"outputs": [],
"source": [
"opals_dataset_3.show_hillshade(resolution=0.5, classification=asprs[\"ground\"])"
Copy link
Member

Choose a reason for hiding this comment

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

Why exactly does the resolution differ on example 3?

"source": [
"# Pdal Example Notebook\n",
"This notebook is a short example of the Pdal utilities provided by `adaptivefiltering`.\n",
"If you have not worked with `adaptivefiltering` before please have a look at the `demo.ipynb`.\n",
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

"outputs": [],
"source": [
"pipeline_1 = adaptivefiltering.load_filter(\"Example_filter/Example_pdal_1.json\")\n",
"pipeline_2 = adaptivefiltering.load_filter(\"Example_filter/Example_pdal_1.json\")\n",
Copy link
Member

Choose a reason for hiding this comment

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

This loads the same filter 3 times!

"metadata": {},
"outputs": [],
"source": [
"dataset_3.show_hillshade(resolution=0.5, classification=asprs[\"ground\"])"
Copy link
Member

Choose a reason for hiding this comment

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

Same question here.

@@ -1,4 +1,6 @@
[pytest]
testpaths = tests jupyter
testpaths = tests jupyter
Copy link
Member

Choose a reason for hiding this comment

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

unnecessary whitespace change

filterwarnings =
ignore::DeprecationWarning
addopts = --ignore jupyter/opals_example.ipynb
Copy link
Member

Choose a reason for hiding this comment

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

Weird whitespace.

@dokempf dokempf closed this Feb 25, 2022
@dokempf dokempf deleted the pdal_opals_notebook branch May 31, 2022 07:47
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

2 participants