-
-
Notifications
You must be signed in to change notification settings - Fork 97
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
Ttc fix new #159
Ttc fix new #159
Conversation
with all the logic to load them back and reconstruct the .stdata file
Reviewer's Guide by SourceryThis pull request implements a significant change to the MeasurementsManager class in the Shake&Tune project. The main goal is to improve memory management by introducing a chunking mechanism for storing measurements. This change allows the system to handle larger datasets more efficiently, particularly for vibration measurements. File-Level Changes
Sequence DiagramsequenceDiagram
participant C as Command
participant MM as MeasurementsManager
participant FS as FileSystem
C->>MM: add_measurement()
MM->>MM: Check if chunk size reached
alt Chunk size reached
MM->>FS: _save_chunk()
MM->>MM: clear_measurements(keep_last=True)
end
C->>MM: save_stdata()
MM->>FS: _reassemble_chunks()
FS-->>MM: Load chunk files
MM->>FS: Save final .stdata file
MM->>FS: Remove chunk files
Tips
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Frix-x - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟡 Documentation: 1 issue found
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
shaketune/helpers/accelerometer.py
Outdated
) | ||
|
||
self._write_process = None | ||
def __del__(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Improve error handling in cleanup process
The current error handling in the cleanup process is very broad. Consider adding more specific exception handling and logging to help diagnose issues that might occur during cleanup. This could help with debugging and ensuring resources are properly released.
def __del__(self): | |
def __del__(self): | |
try: | |
if self._temp_dir.exists(): | |
shutil.rmtree(self._temp_dir) | |
except Exception as e: | |
logging.error(f"Error during cleanup: {e}") | |
finally: | |
if hasattr(self, '_write_process') and self._write_process: | |
self._write_process.terminate() |
# measurements_chunk_size: 15 | ||
# The number of measurements to keep in RAM before writing them to disk. This is useful | ||
# to avoid running out of memory when processing large datasets like for the vibrations | ||
# measurements. If you get Timer Too Close errors, try reducing this value (minimum is 2). | ||
# dpi: 300 | ||
# The resolution of the generated graphs. This usually doesn't need to be changed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (documentation): Consider adding an introductory line for these new configuration options.
It might be helpful to add a brief introductory sentence before listing these new options, explaining that they are additional, advanced configuration settings. This could provide more context for users.
# measurements_chunk_size: 15 | |
# The number of measurements to keep in RAM before writing them to disk. This is useful | |
# to avoid running out of memory when processing large datasets like for the vibrations | |
# measurements. If you get Timer Too Close errors, try reducing this value (minimum is 2). | |
# dpi: 300 | |
# The resolution of the generated graphs. This usually doesn't need to be changed. | |
# Advanced Configuration Options: | |
# The following settings provide additional control over Shake&Tune's behavior. | |
# measurements_chunk_size: 15 | |
# The number of measurements to keep in RAM before writing them to disk. This is useful | |
# to avoid running out of memory when processing large datasets like for the vibrations | |
# measurements. If you get Timer Too Close errors, try reducing this value (minimum is 2). | |
# dpi: 300 | |
# The resolution of the generated graphs. This usually doesn't need to be changed. |
shaketune/helpers/accelerometer.py
Outdated
if keep_last: | ||
self.measurements = [self.measurements[-1]] | ||
else: | ||
self.measurements = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code-quality): Replace if statement with if expression (assign-if-exp
)
if keep_last: | |
self.measurements = [self.measurements[-1]] | |
else: | |
self.measurements = [] | |
self.measurements = [self.measurements[-1]] if keep_last else [] |
shaketune/helpers/accelerometer.py
Outdated
except Exception: | ||
pass # Ignore errors as it's not critical | ||
try: | ||
all_measurements = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (code-quality): Extract code out into method (extract-method
)
Summary by Sourcery
Enhance the MeasurementsManager to support chunking of measurement data, improving memory management and data handling for large datasets. Update related modules to utilize the new chunking feature and ensure proper cleanup of temporary files. Document new configuration options in the README.
Enhancements:
Documentation: