(0) Add camera group data structures #1258
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1258 +/- ##
===========================================
+ Coverage 73.37% 73.41% +0.03%
===========================================
Files 134 135 +1
Lines 23992 24048 +56
===========================================
+ Hits 17605 17655 +50
- Misses 6387 6393 +6
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
roomrys
changed the title
| from qtpy.QtWidgets import QApplication | ||
|
|
||
| from sleap.gui.app import MainWindow | ||
| from sleap.gui.commands import * | ||
|
|
||
|
|
||
| os.environ["QT_QPA_PLATFORM_PLUGIN_PATH"] = QLibraryInfo.location( | ||
| QLibraryInfo.PluginsPath |
|
Converted back to draft due to hacky workaround for version of |
…l/add-camera-group
WalkthroughThis pull request introduces changes to the build scripts, dependencies, and testing environment. It also adds new classes for camera handling in the Changes
TipsChat with CodeRabbit Bot (
|
|
Converting this back to a "Ready for Review" PR to get the coderabbitai feedback. It is definitely not ready to be merged due to the opencv hack described above. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 7
Files ignored due to filter (3)
- environment.yml
- environment_no_cuda.yml
- tests/data/cameras/minimal_session/calibration.toml
Files selected for processing (13)
- .conda/bld.bat (1 hunks)
- .conda/build.sh (1 hunks)
- .conda_mac/build.sh (1 hunks)
- .github/workflows/ci.yml (2 hunks)
- dev_requirements.txt (1 hunks)
- docs/installation.md (1 hunks)
- requirements.txt (1 hunks)
- sleap/io/cameras.py (1 hunks)
- sleap/io/video.py (2 hunks)
- tests/conftest.py (1 hunks)
- tests/fixtures/cameras.py (1 hunks)
- tests/gui/test_app.py (1 hunks)
- tests/io/test_cameras.py (1 hunks)
Files skipped from review due to trivial changes (6)
- dev_requirements.txt
- docs/installation.md
- requirements.txt
- sleap/io/video.py
- tests/conftest.py
- tests/io/test_cameras.py
Additional comments (Suppressed): 8
tests/fixtures/cameras.py (1)
- 1-8: The new hunk introduces a pytest fixture
min_session_calibration_toml_pathwhich returns the path to a calibration file. This fixture can be used in tests that require this specific calibration file. The code looks good and follows best practices for creating pytest fixtures..conda/build.sh (1)
- 12-14: The addition of the OpenCV uninstallation and specific version installation is marked as a hack and untested. This could potentially cause issues if other parts of the code rely on features from newer versions of OpenCV or if the specific version has compatibility issues with other dependencies. Please verify that this change does not break any existing functionality.
.conda/bld.bat (1)
- 12-14: The new code introduces a hack to uninstall all
opencvpackages and install a specific version ofopencv-contrib-python. This is marked as untested. It's crucial to test this change thoroughly, especially in different environments, to ensure it doesn't introduce unexpected behavior or compatibility issues. Also, consider adding a comment explaining why this hack is necessary.+ @REM HACK(LM): (untested) Uninstall all opencv packages and install opencv-contrib-python + for /f "tokens=1" %%a in ('conda list ^| findstr opencv') do pip uninstall %%a -y + pip install "opencv-contrib-python<4.7.0".conda_mac/build.sh (1)
- 12-14: The comment indicates that this change is untested. It's important to test such changes before merging them into the main branch, as they could potentially break the build process. Please verify if these changes have been tested and work as expected.
.github/workflows/ci.yml (2)
96-100: The OpenCV reinstallation step is marked as a hack. This could potentially lead to issues if the specific version of
opencv-contrib-pythonbeing installed is not compatible with other dependencies or parts of the codebase. It's recommended to verify this change and its implications on the overall system.109-115: The description for the test step has been updated from "Test environment" to "Test the code and the environment". Ensure that this change accurately reflects what the test step is doing. If the tests are now also testing the code, make sure that all necessary test cases have been added and are being executed in this step.
tests/gui/test_app.py (1)
- 1-14: The new hunk introduces a workaround to ensure that the libraries
sleap-anipose,aniposelib, andimgauguse headless OpenCV. This is done by setting theQT_QPA_PLATFORM_PLUGIN_PATHenvironment variable to the location of the Qt plugins path. While this is a valid solution, it's important to note that this might have implications on other parts of the codebase that rely on these libraries or theQT_QPA_PLATFORM_PLUGIN_PATHenvironment variable. Please verify that this change doesn't introduce any unexpected behavior elsewhere in the application.sleap/io/cameras.py (1)
- 24-30: The equality check for
Camcorderobjects only checks the attributes of theCamcorderclass itself and does not take into account the attributes of theCameraorFisheyeCameraobjects stored in thecameraattribute. If thecameraattribute's properties should be considered in the equality check, you may need to revise this method.
| @@ -9,6 +9,9 @@ export PIP_IGNORE_INSTALLED=False | |||
| # https://docs.conda.io/projects/conda-build/en/stable/user-guide/wheel-files.html) | |||
| pip install --no-cache-dir -r ./requirements.txt | |||
|
|
|||
| # HACK(LM): (untested) Uninstall all opencv packages and install opencv-contrib-python | |||
| conda list | grep opencv | awk '{system("pip uninstall " $1 " -y")}' | |||
There was a problem hiding this comment.
Using system() to execute shell commands can be risky due to potential command injection vulnerabilities. Although in this case, the risk is low because the input to system() is not user-controlled, it's still a good practice to avoid using system() when possible. Consider using a safer alternative for executing shell commands.
| conda list | grep opencv | awk '{system("pip uninstall " $1 " -y")}' | ||
| pip install "opencv-contrib-python<4.7.0" |
There was a problem hiding this comment.
The use of conda list | grep opencv | awk '{system("pip uninstall " $1 " -y")}' to uninstall all OpenCV packages is a bit risky. It assumes that all packages with 'opencv' in their name are related to OpenCV and should be uninstalled, which might not always be the case. Also, it's using pip to uninstall packages installed by conda, which could lead to inconsistencies in the environment. Consider using conda uninstall --force opencv instead to ensure only the opencv package is removed and there are no conflicts between conda and pip.
- conda list | grep opencv | awk '{system("pip uninstall " $1 " -y")}'
+ conda uninstall --force opencv
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Files selected for processing (2)
- sleap/io/cameras.py (1 hunks)
- tests/gui/test_app.py (1 hunks)
Additional comments (Suppressed): 1
tests/gui/test_app.py (1)
- 1-14: The environment variable
QT_QPA_PLATFORM_PLUGIN_PATHis being set to the location of the Qt plugins path. This is a workaround for an issue with headless OpenCV, as mentioned in the comment. Ensure that this does not interfere with other parts of the application that might be using Qt or OpenCV. Also, consider moving this setup code to a separate function or module to improve modularity and maintainability.
…l/add-camera-group
Description
Types of changes
Does this address any currently open issues?
[list open issues here]
Outside contributors checklist
Thank you for contributing to SLEAP!
❤️
Summary by CodeRabbit
New Features:
CamcorderandCameraClusterclasses to thesleap.io.camerasmodule for improved camera handling.requirements.txt.Improvements:
Documentation:
docs/installation.mdwith instructions for setting up the development environment usingmambaandenvironment_mac.yml.Tests:
CamcorderandCameraClusterclasses intests/io/test_cameras.py.tests/gui/test_app.pyby setting theQT_QPA_PLATFORM_PLUGIN_PATHenvironment variable.