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

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

willsanddrew
Copy link

@willsanddrew willsanddrew commented Feb 6, 2023

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
Copy link

swell-d commented Dec 1, 2023

👍

@dancergraham dancergraham added the enhancement New feature or request label Feb 9, 2024
@gredin
Copy link

gredin commented Apr 11, 2024

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

@dancergraham
Copy link
Collaborator

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.

To explain the checks and launch the tests
Copy link
Collaborator

@dancergraham dancergraham left a comment

Choose a reason for hiding this comment

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

ahh the tests fail - can you have a look at why @gredin ?

@dancergraham
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Collaborator

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
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants