davidcaron / pye57

Read and write e57 point clouds from Python
MIT License
71 stars 42 forks source link

Fix bug preventing raw scans without optional elements from being written #36

Open willsanddrew opened 1 year ago

willsanddrew commented 1 year ago

As per the standards for e57 files, several child elements (listed below) are optional, but write_scan_raw fails if they aren't present in the data. The calls to getattr aren't returning the default values but instead are throwing exceptions. To fix this I've refactored these elements to be written out only if they're present in the source data, using the "if [] in data" that other optional elements use.

temperature relativeHumidity atmosphericPressure acquisitionStart_dateTimeValue acquisitionStart_isAtomicClockReferenced acquisitionEnd_dateTimeValue acquisitionEnd_isAtomicClockReferenced

Here's a code snippet that demonstrates the issue, using pump.e57 from the libe57 website:


import pye57

e57 = pye57.E57("pump.e57")
data = e57.read_scan_raw(0)
e57_write = pye57.E57("pump_out.e57", mode="w")
e57_write.write_scan_raw(data, scan_header=e57.get_header(0))
swell-d commented 1 year ago

👍

gredin commented 7 months ago

@davidcaron Is there anything preventing you from merging this pull request? And if so, can I help?

dancergraham commented 7 months ago

Hello this looks pretty easy to do - I should be able to merge it! Do you have a reference to those being optional fields? Adding a test using a very small e57 test file (e.g. heavily subsampled / cut down using cloud compare ?) would be awesome too if possible.

dancergraham commented 7 months ago

ahh got it - it looks like the required and optional fields are given in table 13 of this document https://cdn.standards.iteh.ai/samples/102656/ef2c1f2f33344506946c5ad65aad924f/ASTM-E2807-11-2019-.pdf

willsanddrew commented 7 months ago

I haven't had time to fully dig into the issue yet, but at a quick glance if looks like the test test_copy_file should be updated to pass if any of those seven optional attributes aren't present. I think putting those checks for optional attributes in a try/except block looking for this exception:pye57.libe57.E57Exception: E57 element path well formed but not defined (ErrorPathUndefined) should catch the error.

Additionally, the asset statements for temperature relativeHumidity and atmosphericPressure on that test aren't currently doing anything, as they're comparing header_written to header_written when the rest of the assert statements are comparing header_written to header.

dancergraham commented 7 months ago

Hah yes good spot with the header written tests! Yes updating the test to ensure it raises an appropriate exception for data we know to be missing from the test file sounds perfect.

dancergraham commented 3 months ago

Just coming back to this - the test file does include temperature, relativeHumidity and atmospheric pressure so I'm not sure why the tests failed. I will try again from a new branch - because you are working on master, not from a branch, I cannot modify your pull request