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

Release 0.0.2 of LiDAR tiling workflow #12

Merged
merged 182 commits into from
Mar 20, 2024
Merged

Release 0.0.2 of LiDAR tiling workflow #12

merged 182 commits into from
Mar 20, 2024

Conversation

iannesbitt
Copy link
Contributor

At present the workflow is contained in the pdgpoints.pipeline.Pipeline class, which can be executed with Pipeline.run(). In constructing it this way I hoped to make it easy to parallelize in the future, either with threading or a Ray/Slurm instance.
Certain steps of this workflow are executed with LAStools binary. LAStools is multithreading-enabled but I doubt that functionality can be scaled across machines easily.

This workflow contains 5 steps:

  1. Use las2las to ensure LiDAR file (LAS, LAZ, or other major filetype) has CRS information in WKT string
  2. Use lasinfo to read WKT string, write to file
  3. Use las2las to rewrite LiDAR file with corrected header, write WKT string from file to header. Write 8 bit intensity values to 16 bit R, G, and B channels if necessary.
  4. Use py3dtiles.convert._Convert class to tile dataset, use WKT string from file to input CRS info
  5. Use py3dtiles.merger.merge_from_files() to merge created tileset with existing tilesets

Test dataset is the Olympic Jumping Complex in Lake Placid, NY.

@iannesbitt iannesbitt changed the title Release 0.0.1 of LiDAR tiling workflow Release 0.0.2 of LiDAR tiling workflow Jul 5, 2023
@iannesbitt iannesbitt modified the milestones: 0.0.1, 0.0.2 Jul 5, 2023
@iannesbitt
Copy link
Contributor Author

iannesbitt commented Jul 5, 2023

@robyngit I have finished most of the requested changes from your review. A brief summary of what's changed:

  • Swapped getopt with argparse (Swap getopt with argparse #19)
  • Improved path management by converting all relevant path instantiations to pathlib.Path objects (Improve path management #22) and removing relative references to home directory
  • Standardized use of logging (Standardize use of logging #23)
    • log config file location is defined in pdgpoints.defs.LOGCONFIG
    • json configuration is loaded to dict in pdgpoints.defs.LOGGING_CONFIG
    • pdgpoints.__init__ calls dictConfig(LOGGING_CONFIG`) at runtime
    • each function that requires logging uses L = getLogger(__name__)
  • Corrected geoid height issue (Point cloud shows up significantly above or below terrain surface #14)
    • WKT strings now parse correctly (Parse WKT strings effectively #24)
    • CRS and VRS are extracted from WKT in file header and assumed correct by default
      • user can specify a geoid to fall back to if VRS cannot be found in LAS file
    • dataset centroids are found by writing x and y of every 10,000th point to file, then using pandas to get mean of each variable set
    • centroid x and y are converted to lat and lon with pyproj.Transformer
    • query NOAA API for point source geoid height (using either detected or user supplied VRS) at lat and lon calculated and set vertical adjustment factor to returned value

I've decided to bump the version to 0.0.2 given the many breaking changes.

What I still have yet to implement from your review:

  • Automated tests #26 (self explanatory)

  • Improve path management #22

    In parts of viz-points, the input path is used to construct the output path

    I will need some help resolving this as I am currently unfamiliar with node-based file management. Shouldn't be too difficult to fix based on how it's currently coded, however.

  • Increased level of detail on zoom
    I think this must be a py3dtiles issue because I've noticed that their merge function tends to break the usability of tiled datasets (see for example Merged point cloud disappears when viewing from certain angles #9). If you set debugWireframe: true in Cesium.Cesium3DTileset you can see that the bounding box extends far beyond the boundaries of the points when merged, but is normal when simply tiled. I think this means they have an error in their bounding volume calculation that causes some issues with the camera and tile prioritization (apologies if my vocabulary isn't quite correct here). I've tried investigating but I think I'm missing some knowledge about how Cesium expects to read the tile hierarchy in order to assess what's actually happening in py3dtiles. We should be able to resolve this easily if we bring our version of py3dtiles in line with Oslandia's. I'm not sure when I can get around to this but it's on my list of big to-do items.

  • Manually testing in Cesium during development
    My cesium.js file is included below:

    function start(){
    
      Cesium.Ion.defaultAccessToken = "TOKEN";
    
      var tileset = new Cesium.Cesium3DTileset({
        url: "3dtiles/tileset.json",
        skipLevelOfDetail: true,
        debugWireframe: false,
        });
    
      tileset.style = new Cesium.Cesium3DTileStyle({
        pointSize: 1.5,
        show: true
      });
    
      tileset.pointCloudShading.attenuation = true;
      viewer.scene.primitives.add(tileset);
    
      window.zoom_to_me = function(){
        viewer.zoomTo(tileset);
      }
    
      viewer.scene.globe
      tileset.readyPromise.then(zoom_to_me).otherwise(error => { console.log(error) });
    
    }
    
    start()
  • Record keeping #27
    This is a great idea and I will be implementing it as I work to make a stable version of this program. It seems like something that I can implement alongside Create config system to store per-project processing info #25.

With your approval I will move to merge this to main.

@robyngit
Copy link
Member

@iannesbitt the changes you made look great! 🎉 I wasn't able to test everything because I got an error during installation:

Expand to see error
(viz-points) ➜  viz-points git:(develop) pip install .                        
Processing /Users/rtb/git/viz-points
  Preparing metadata (setup.py) ... error
  error: subprocess-exited-with-error
  
  × python setup.py egg_info did not run successfully.
  │ exit code: 1
  ╰─> [29 lines of output]
      Traceback (most recent call last):
        File "/usr/local/anaconda3/envs/viz-points/lib/python3.9/logging/config.py", line 564, in configure
          handler = self.configure_handler(handlers[name])
        File "/usr/local/anaconda3/envs/viz-points/lib/python3.9/logging/config.py", line 745, in configure_handler
          result = factory(**kwargs)
        File "/usr/local/anaconda3/envs/viz-points/lib/python3.9/logging/handlers.py", line 153, in __init__
          BaseRotatingHandler.__init__(self, filename, mode, encoding=encoding,
        File "/usr/local/anaconda3/envs/viz-points/lib/python3.9/logging/handlers.py", line 58, in __init__
          logging.FileHandler.__init__(self, filename, mode=mode,
        File "/usr/local/anaconda3/envs/viz-points/lib/python3.9/logging/__init__.py", line 1146, in __init__
          StreamHandler.__init__(self, self._open())
        File "/usr/local/anaconda3/envs/viz-points/lib/python3.9/logging/__init__.py", line 1175, in _open
          return open(self.baseFilename, self.mode, encoding=self.encoding,
      FileNotFoundError: [Errno 2] No such file or directory: '/home/nesbitt/log/viz-points/pdgpoints.log'
      
      The above exception was the direct cause of the following exception:
      
      Traceback (most recent call last):
        File "<string>", line 2, in <module>
        File "<pip-setuptools-caller>", line 34, in <module>
        File "/Users/rtb/git/viz-points/setup.py", line 2, in <module>
          from pdgpoints import _version
        File "/Users/rtb/git/viz-points/pdgpoints/__init__.py", line 4, in <module>
          dictConfig(LOGGING_CONFIG)
        File "/usr/local/anaconda3/envs/viz-points/lib/python3.9/logging/config.py", line 809, in dictConfig
          dictConfigClass(config).configure()
        File "/usr/local/anaconda3/envs/viz-points/lib/python3.9/logging/config.py", line 571, in configure
          raise ValueError('Unable to configure handler '
      ValueError: Unable to configure handler 'debugfile'
      [end of output]
  
  note: This error originates from a subprocess, and is likely not a problem with pip.
error: metadata-generation-failed

× Encountered error while generating package metadata.
╰─> See above for output.

note: This is an issue with the package mentioned above, not pip.
hint: See above for details.

The only other comment I have at this point is super minor - I think it's more typical to have the test scripts & test data at the root level of the repo rather than inside the pdgpoints, but perhaps there an argument for doing it this way?

@iannesbitt
Copy link
Contributor Author

@robyngit The error was an oversight on my part...I was logging in my home dir because I don't have sudo privileges on datateam. If you are running on datateam and don't have permission to create /var/log/viz-points/, you will have to edit the two filename fields in pdgpoints/log/config.json to point to your own log directory. Otherwise logs will go to /var/log/viz-points/... as they are now configured for.

As for the test scripts and data, the reason I did that is because I'm pretty sure the manifest will only include files (such as the two test LAS files) when they exist inside a module. So maybe it makes sense to make a test/ module?

@robyngit
Copy link
Member

Logging

@iannesbitt I'm not sure of the best way to set up logging, but it does seem like an atypical pattern to require a logging config file to exist and be properly set up before the user can install or import a python package. What do you think? Is there a way to enable users to configure the logging after the package has been installed and imported?

Tests

I think your release is good as-is, without changing your test set up, but just to continue the discussion:

If I understand your proposed solution correctly, I do think that is a more common set up. So use a file structure like the one below and then add recursive-include tests * to the manifest?

viz-points/
├── pdgpoints/
│   ├── __init__.py
│   ├── ... etc
├── tests/
│   ├── __init__.py
│   ├── test.py
│   ├── data/
│       ├── README.md
│       ├── lp.png
│       ├── lp_jumps_e.laz
│       ├── lp_jumps_w.laz
├── ... etc

BTW, also not necessary right away, but you could also include other dev dependencies you're using in setup.py, like:

extras_require={
    'dev': [
        'sphinx',
    ]
}

@iannesbitt
Copy link
Contributor Author

Merging to main as all immediately relevant points have been addressed, and others have issues tracking them for a future release.

@iannesbitt iannesbitt merged commit 52c0e2c into main Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants