Conversation
… and cleanup outdated docstrings.
|
Review these changes at https://app.gitnotebooks.com/AlphaSphereDotAI/visualizr/pull/83 |
Reviewer's GuideThis PR focuses on code style and configuration cleanup across the project, standardizing docstrings, reflowing comments and asserts, reorganizing imports, tweaking the CI bootstrap sequence, and pruning obsolete config files. Flow Diagram for Updated CI Bootstrap Sequence in qodana.yamlgraph TD
A[Start] --> B["rm uv.lock"];
B --> C["rm -rf .idea"];
C --> D["export UV_CACHE_DIR=/data/cache"];
D --> E["export CONDA_PREFIX=/opt/miniconda3"];
E --> F["conda config --add channels defaults"];
F --> G["conda install -y python=3.10"];
G --> H["pip install uv"];
H --> I["uv pip sync pyproject.toml --python 3.10"];
I --> J[End];
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughThis update primarily involves configuration and documentation changes, including the deletion and simplification of several linter and formatter config files, updates to project dependency and linter versions, and minor code comment and type hint improvements. Several Python methods now explicitly declare global variables. No major functional or control flow changes are introduced to the codebase. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FaceEnhancer
User->>FaceEnhancer: Call enhancer_generator_no_len(method)
alt method is valid ("gfpgan", "RestoreFormer", "codeformer")
FaceEnhancer->>FaceEnhancer: Proceed with enhancement
else method is invalid
FaceEnhancer->>User: Raise ValueError
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
for more information, see https://pre-commit.ci
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @MH0386, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Based on the title 'ci' and the changes included, this pull request appears to focus on general maintenance. It includes a minor adjustment to the CI configuration and several small code cleanups, such as removing outdated comments and improving code formatting, along with adding a clarifying comment in one of the model files. Without a detailed description, the primary goal seems to be maintaining the codebase and potentially improving the CI process.
Highlights
- CI Configuration Update: Adjusted the order of cleanup commands (
rm uv.lock,rm -rf .idea) within thebootstrapsection of theqodana.yamlconfiguration file. - Codebase Cleanup: Removed comments referencing external source code links in
diffusion/base.pyandmodel/blocks.py. Also, reformatted some comments andassertstatements acrossmodel/blocks.pyandmodel/unet.pyfor improved readability. - Model Code Clarity: Added explanatory comments to the
forwardmethod inmodel/seq2seq.pyto clarify the purpose of theinitial_codeanddirection_codevariables.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configureGemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
|
Here's the code health analysis summary for commits Analysis Summary
|
There was a problem hiding this comment.
Code Review
This pull request primarily focuses on cleaning up code comments and documentation, updating CI configuration for Qodana, and removing obsolete configuration files. The changes generally improve code hygiene and CI pipeline reliability. Key areas for consideration include the removal of attribution links in src/visualizr/diffusion/base.py and src/visualizr/model/blocks.py; it's worth ensuring these removals are appropriate if the code still significantly draws from the original sources.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @MH0386 - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/visualizr/dataset.py:48` </location>
<code_context>
- )
-
- self.data = []
- for db_name in ["VoxCeleb2", "HDTF"]:
- db_png_path = os.path.join(frame_jpgs, db_name)
- for clip_name in tqdm(os.listdir(db_png_path)):
</code_context>
<issue_to_address>
Avoid shadowing the `db_name` parameter.
Rename the loop variable to avoid confusion and potential errors.
</issue_to_address>
### Comment 2
<location> `src/visualizr/dataset.py:152` </location>
<code_context>
- if min_len < self.window_size * self.video_fps + 5:
- continue
-
- print("Db count:", len(self.data))
-
- def get_single_image(self, image_path):
</code_context>
<issue_to_address>
No items are ever appended to `self.data`.
Consider adding `self.data.append(item_dict)` after constructing and validating `item_dict` to ensure items are stored as intended.
</issue_to_address>
### Comment 3
<location> `qodana.yaml:5` </location>
<code_context>
linter: jetbrains/qodana-python:2025.1
bootstrap: |
+ rm uv.lock
+ rm -rf .idea
export UV_CACHE_DIR=/data/cache
export CONDA_PREFIX=/opt/miniconda3
</code_context>
<issue_to_address>
Removing `.idea` before redirecting logs will break log files.
Removing the `.idea` directory before redirecting logs to `.idea/output.log` will cause the redirection to fail. Remove `.idea` after logging, or recreate the directory before redirecting logs.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| ) | ||
|
|
||
| self.data = [] | ||
| for db_name in ["VoxCeleb2", "HDTF"]: |
There was a problem hiding this comment.
issue: Avoid shadowing the db_name parameter.
Rename the loop variable to avoid confusion and potential errors.
| continue | ||
|
|
||
| print("Db count:", len(self.data)) | ||
|
|
There was a problem hiding this comment.
issue (bug_risk): No items are ever appended to self.data.
Consider adding self.data.append(item_dict) after constructing and validating item_dict to ensure items are stored as intended.
|
|
||
| class LatentDataLoader(object): | ||
| def __init__( | ||
| self, |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Use f-string instead of string concatenation [×5] (
use-fstring-for-concatenation) - Low code quality found in LatentDataLoader.__init__ - 16% (
low-code-quality)
Explanation
The quality score for this function is below the quality threshold of 25%.
This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines. - Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.
|
|
||
| total_lmd_obj = [] | ||
| for i, line in enumerate(lmd_lines): | ||
| # Split the coordinates and filter out any empty strings |
There was a problem hiding this comment.
suggestion (code-quality): Remove unnecessary calls to enumerate when the index is not used (remove-unused-enumerate)
| for i, line in enumerate(lmd_lines): | |
| for line in lmd_lines: |
| return distances | ||
|
|
There was a problem hiding this comment.
suggestion (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)
| distances = np.linalg.norm(forehead_center - chin_bottom, axis=1, keepdims=True) | |
| return distances | |
| return np.linalg.norm(forehead_center - chin_bottom, axis=1, keepdims=True) |
Qodana for Python604 new problems were found
☁️ View the detailed Qodana report Contact Qodana teamContact us at qodana-support@jetbrains.com
|
…kflow to disable auto-fixes and PR pushes.
…kflow to disable auto-fixes and PR pushes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/visualizr/dataset.py (2)
23-36:db_nameparameter shadowed by loop variable
Same issue pointed out previously persists; rename the loop variable to avoid confusion.
47-152:⚠️ Potential issueCritical: items never added to
self.data
self.dataremains an empty list because the inner loop buildsitem_dictbut never appends it.
Down-stream__len__and__getitem__will malfunction.@@ if min_len < self.window_size * self.video_fps + 5: continue + + # finally store the validated clip + self.data.append(item_dict)🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 48-48: Redefining argument with the local name 'db_name'
(R1704)
[refactor] 51-51: Consider using '{}' instead of a call to 'dict'.
(R1735)
🧹 Nitpick comments (2)
src/visualizr/dataset.py (1)
174-177: Unused loop indexi
iis not referenced inside the loop; rename to_or dropenumerate.- for i, line in enumerate(lmd_lines): + for line in lmd_lines:🧰 Tools
🪛 Ruff (0.11.9)
175-175: Loop control variable
inot used within loop bodyRename unused
ito_i(B007)
qodana.yaml (1)
8-9: Prevent log overwrites by using append or separate files.Both
conda config --add… > output.logandconda install… > output.logcurrently overwrite the same file, losing earlier logs. Consider one of the following:
- Use
>> output.logto append each command’s output.- Redirect each command to its own log file (e.g.,
config.logandinstall.log).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/workflows/code_analysis.yaml(1 hunks).trunk/configs/.yamllint.yaml(0 hunks).trunk/configs/pyrightconfig.json(0 hunks)qodana.yaml(1 hunks)src/visualizr/dataset.py(1 hunks)src/visualizr/diffusion/base.py(0 hunks)src/visualizr/model/blocks.py(1 hunks)src/visualizr/model/seq2seq.py(1 hunks)src/visualizr/model/unet.py(1 hunks)
💤 Files with no reviewable changes (3)
- src/visualizr/diffusion/base.py
- .trunk/configs/pyrightconfig.json
- .trunk/configs/.yamllint.yaml
🧰 Additional context used
🪛 Ruff (0.11.9)
src/visualizr/dataset.py
175-175: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
🪛 Pylint (3.3.7)
src/visualizr/dataset.py
[refactor] 13-13: Class 'LatentDataLoader' inherits from object, can be safely removed from bases in python3
(R0205)
[refactor] 13-13: Too many instance attributes (11/7)
(R0902)
[refactor] 14-14: Too many arguments (13/5)
(R0913)
[refactor] 14-14: Too many positional arguments (13/5)
(R0917)
[refactor] 14-14: Too many local variables (24/15)
(R0914)
[refactor] 48-48: Redefining argument with the local name 'db_name'
(R1704)
[refactor] 51-51: Consider using '{}' instead of a call to 'dict'.
(R1735)
[refactor] 14-14: Too many statements (62/50)
(R0915)
[refactor] 201-201: Too many local variables (18/15)
(R0914)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Qodana
- GitHub Check: Mergify Merge Protections
- GitHub Check: Qodana
🔇 Additional comments (4)
src/visualizr/model/unet.py (1)
29-35: Comment-only tweak acknowledgedPure comment re-flow, no behavioural impact.
.github/workflows/code_analysis.yaml (1)
66-68: Confirm intention to disable Qodana auto-fixes
--apply-fixesandpush-fixesare now commented out.
If this was accidental, CI will still surface problems but never amend them, so technical debt could silently accumulate.src/visualizr/model/blocks.py (1)
412-412: Docstring shrinkage looks fineConcise wording, behaviour unchanged.
src/visualizr/model/seq2seq.py (1)
148-154: Comment addition improves intent clarityNo functional diff; LGTM.
| rm uv.lock | ||
| rm -rd .idea |
There was a problem hiding this comment.
Make cleanup commands idempotent and error-tolerant.
Without force flags, rm uv.lock will error if the file is missing, and rm -rd .idea may fail or behave unexpectedly (the -d flag only removes empty directories). This can break the CI bootstrap.
Apply this diff:
- rm uv.lock
- rm -rd .idea
+ rm -f uv.lock
+ rm -rf .idea📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rm uv.lock | |
| rm -rd .idea | |
| rm -f uv.lock | |
| rm -rf .idea |
🤖 Prompt for AI Agents
In qodana.yaml around lines 4 to 5, the cleanup commands use `rm uv.lock` and
`rm -rd .idea`, which can fail if the file or directory does not exist or if the
directory is not empty. To fix this, add the `-f` flag to `rm uv.lock` to avoid
errors if the file is missing, and replace `rm -rd .idea` with `rm -rf .idea` to
forcefully and recursively remove the directory regardless of its contents,
making the cleanup commands idempotent and error-tolerant.
…ble auto-fixes with PR pushes in workflow.
for more information, see https://pre-commit.ci
…ity of global variable definitions in `encoder.py`.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
src/visualizr/networks/styledecoder.py (1)
588-594: 🛠️ Refactor suggestionRemove
globalusage; keepskip&skip_flowlocal toforward.Exposing intermediate tensors via the
globalnamespace:
- Breaks re-entrancy & thread-safety – multiple model instances running in dataloader workers will trample each other.
- Makes debugging harder because the lifecycle of these tensors is no longer explicit.
- Creates silent memory leaks on long-running processes.
A self-contained approach keeps the two variables local and initialises them before the loop:
- global skip_flow, skip + skip_flow: torch.Tensor | None = None + skip: torch.Tensor | None = NoneAll subsequent references already occur inside this scope, so no further change is needed.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 589-589: Too many local variables (16/15)
(R0914)
src/visualizr/utils.py (1)
150-160: 🛠️ Refactor suggestion
global frame_end, audio_drivenis unnecessary and harmfulBoth names are only consumed inside
main; declaring themglobal:
- Pollutes the module namespace.
- Prevents concurrent calls to
main(e.g., fromgradiothreads) from running safely.- Adds hidden coupling with external code – but no other function actually imports them.
Simply drop the declaration – nothing else in this file relies on these globals.
- global frame_end, audio_driven
🧹 Nitpick comments (1)
src/visualizr/face_sr/face_enhancer.py (1)
47-48: Early validation is welcome, but make it case-insensitive & DRYNice to see the guard added before heavy init.
Two small tweaks would make it more robust and avoid future duplication with thematchblock:
- Normalize the input once (
method = method.lower()), then keepvalid_methodsin one tuple that is reused for both the upfront check and thematch.- Update the
caselabels to the same lower-case strings, eliminating the mixed-case"RestoreFormer".Example sketch:
- if method not in ["gfpgan", "RestoreFormer", "codeformer"]: - raise ValueError(f"Wrong model version {method}.") + method = method.lower() + valid_methods = ("gfpgan", "restoreformer", "codeformer") + if method not in valid_methods: + raise ValueError(f"Unknown face-enhancement method: {method}")This prevents surprises such as callers passing
"RestoreFormer"vs"restoreformer"and centralises the allowed set.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
qodana.yaml(1 hunks)src/visualizr/diffusion/base.py(1 hunks)src/visualizr/face_sr/face_enhancer.py(1 hunks)src/visualizr/model/diffusion.py(1 hunks)src/visualizr/model/seq2seq.py(2 hunks)src/visualizr/model/unet_autoenc.py(1 hunks)src/visualizr/networks/encoder.py(1 hunks)src/visualizr/networks/styledecoder.py(1 hunks)src/visualizr/utils.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/visualizr/model/diffusion.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/visualizr/diffusion/base.py
- src/visualizr/model/seq2seq.py
- qodana.yaml
🧰 Additional context used
🪛 Ruff (0.11.9)
src/visualizr/model/unet_autoenc.py
221-221: Loop control variable j not used within loop body
Rename unused j to _j
(B007)
240-240: Loop control variable j not used within loop body
Rename unused j to _j
(B007)
285-289: Use ternary operator time_emb = None if time_emb is None else self.time_embed(time_emb) instead of if-else-block
(SIM108)
🪛 GitHub Check: Qodana for Python
src/visualizr/model/unet_autoenc.py
[warning] 1-1: Inconsistent line separators
Line separators in the current file (\r\n) differ from the project defaults (\n)
🪛 Pylint (3.3.7)
src/visualizr/model/unet_autoenc.py
[refactor] 124-128: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 130-130: Too many arguments (9/5)
(R0913)
[refactor] 130-130: Too many positional arguments (9/5)
(R0917)
[refactor] 130-130: Too many local variables (29/15)
(R0914)
[refactor] 130-130: Too many branches (18/12)
(R0912)
[refactor] 130-130: Too many statements (56/50)
(R0915)
[refactor] 273-273: Too few public methods (1/2)
(R0903)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Mergify Merge Protections
| def noise_to_cond(self, noise: Tensor): | ||
| raise NotImplementedError() | ||
| # assert self.conf.noise_net_conf is not None | ||
| # return self.noise_net.forward(noise) | ||
There was a problem hiding this comment.
noise_to_cond() now always raises – forward pass will crash
forward() calls self.noise_to_cond(noise) when noise is not None (l.154-157).
With the current stub, any training code that supplies noise will hit NotImplementedError.
Either implement the mapping or delete the call path (and the argument) to avoid a runtime failure.
- def noise_to_cond(self, noise: Tensor):
- raise NotImplementedError()
+ def noise_to_cond(self, noise: Tensor):
+ # Example: simple MLP, replace with real logic
+ return self.noise_net(noise)If the feature is truly obsolete, remove the parameter check in forward instead.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def noise_to_cond(self, noise: Tensor): | |
| raise NotImplementedError() | |
| # assert self.conf.noise_net_conf is not None | |
| # return self.noise_net.forward(noise) | |
| def noise_to_cond(self, noise: Tensor): | |
| # Example: simple MLP, replace with real logic | |
| return self.noise_net(noise) |
🤖 Prompt for AI Agents
In src/visualizr/model/unet_autoenc.py around lines 83 to 87, the
noise_to_cond() method currently raises NotImplementedError, causing a crash
when forward() calls it if noise is provided. To fix this, either implement the
noise_to_cond() method to properly map noise to conditioning or remove the noise
argument and the call to noise_to_cond() in forward() to prevent runtime errors.
If the noise feature is obsolete, also remove the noise parameter check in
forward() to avoid calling the unimplemented method.
… refine dependency markers for CUDA compatibility.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.trunk/trunk.yaml (1)
16-19: Confirm disabling formatters and linters.The removal of
blackandisort(alongside existingpyrightandyamllint) from the enabled list could impact code formatting and type checking in your workflow. Please verify that no downstream processes or pre-commit hooks depend on these tools and update any documentation or scripts accordingly.Consider adding an inline comment explaining the rationale for disabling these formatters to aid future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
.idea/codeStyles/codeStyleConfig.xml(1 hunks).trunk/configs/.isort.cfg(0 hunks).trunk/configs/.markdownlint.yaml(0 hunks).trunk/configs/ruff.toml(1 hunks).trunk/configs/taplo.toml(0 hunks).trunk/trunk.yaml(1 hunks)pyproject.toml(1 hunks)
💤 Files with no reviewable changes (3)
- .trunk/configs/.markdownlint.yaml
- .trunk/configs/.isort.cfg
- .trunk/configs/taplo.toml
✅ Files skipped from review due to trivial changes (3)
- .idea/codeStyles/codeStyleConfig.xml
- .trunk/configs/ruff.toml
- pyproject.toml
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Trunk Check
- GitHub Check: Qodana
- GitHub Check: Trunk Code Quality
- GitHub Check: Mergify Merge Protections
- GitHub Check: Qodana
- GitHub Check: Trunk Code Quality
🔇 Additional comments (1)
.trunk/trunk.yaml (1)
37-38: Approve new pre-commit and pre-push actions.Adding
trunk-check-pre-push-alwaysandtrunk-check-pre-commitstrengthens your CI by enforcing checks at earlier stages. This aligns well with best practices for preventing regressions.
…logging paths, and exclusion rules; add type hinting in `dataset.py`.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/visualizr/dataset.py (2)
49-50: Loop variable shadows constructor parameterdb_nameUsing the same identifier for two different concepts is confusing and risks bugs if the parameter were ever used. Rename the loop variable (e.g.
dataset_name) and, if useful, store it initem_dictfor later tracing.-for db_name in ["VoxCeleb2", "HDTF"]: - db_png_path = os.path.join(frame_jpgs, db_name) +for dataset_name in ["VoxCeleb2", "HDTF"]: + db_png_path = os.path.join(frame_jpgs, dataset_name)
145-153: self.data never populated – Dataset length is always zero
__len__and__getitem__rely onself.data, but no item is appended after all validations pass, so every call will either return0or raise anIndexError.
Add the append once the sample clears themin_lengate:if min_len < self.window_size * self.video_fps + 5: continue - # (nothing is stored – bug) + # All checks passed – persist the sample + self.data.append(item_dict)
🧹 Nitpick comments (2)
src/visualizr/dataset.py (2)
176-176: Unused loop index variable
iis never referenced inside the loop; replace it with_to signal intentional discard and silence linters.-for i, line in enumerate(lmd_lines): +for _, line in enumerate(lmd_lines):
14-14: Redundant ‘object’ base classIn Python 3, explicitly inheriting from
objectis unnecessary. Removing it cuts noise and appeases style tools.-class LatentDataLoader(object): +class LatentDataLoader:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.idea/sonarlint.xml(1 hunks)qodana.yaml(1 hunks)src/visualizr/dataset.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .idea/sonarlint.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- qodana.yaml
🧰 Additional context used
🪛 Ruff (0.11.9)
src/visualizr/dataset.py
176-176: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
🪛 Pylint (3.3.7)
src/visualizr/dataset.py
[refactor] 14-14: Class 'LatentDataLoader' inherits from object, can be safely removed from bases in python3
(R0205)
[refactor] 14-14: Too many instance attributes (11/7)
(R0902)
[refactor] 15-15: Too many arguments (13/5)
(R0913)
[refactor] 15-15: Too many positional arguments (13/5)
(R0917)
[refactor] 15-15: Too many local variables (24/15)
(R0914)
[refactor] 49-49: Redefining argument with the local name 'db_name'
(R1704)
[refactor] 15-15: Too many statements (62/50)
(R0915)
[refactor] 202-202: Too many local variables (18/15)
(R0914)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Trunk Check
- GitHub Check: Trunk Code Quality
- GitHub Check: Check Dependency Compatibility
- GitHub Check: Qodana
- GitHub Check: Mergify Merge Protections
- GitHub Check: Qodana
|
Hi @MH0386, Your PR is in conflict and cannot be merged. |
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/visualizr/dataset.py (2)
49-49: Avoid shadowing the constructor parameterdb_name.The loop variable
db_namehides the constructor argument with the same name, which makes the original value inaccessible inside the loop and is error-prone.
Rename the loop variable, e.g. todataset_name.
48-153:self.datais never populated – the dataset will always be empty.After constructing and validating
item_dict, it is never appended toself.data.
As a result,__len__will return 0 and__getitem__will fail.@@ item_dict["frame_count"] = min_len item_dict["hubert_obj"] = item_dict["hubert_obj"][ :, : min_len * 2, : ] if min_len < self.window_size * self.video_fps + 5: continue + + # ✅ Store the sample + self.data.append(item_dict) @@ print("Db count:", len(self.data))
🧹 Nitpick comments (3)
src/visualizr/dataset.py (3)
52-53: Use a precise type hint (Dict[str, Any]) instead of the unparameterisedDict.-from typing import Dict +from typing import Dict, Any ... -item_dict: Dict = {} +item_dict: Dict[str, Any] = {}Using
Dictwithout type parameters defeats the purpose of the hint and triggerstypingwarnings.
160-168: Minor API & naming improvements forget_multiple_ranges.
- The parameter name
listsshadows the built-inlisttype.- Guard clauses validate tuple shape, but not range ordering or bounds.
- You can eliminate the intermediate
extracted_elementsvariable for clarity.-def get_multiple_ranges(self, lists, multi_ranges): +def get_multiple_ranges(self, sequence, multi_ranges): @@ - extracted_elements = [lists[start:end] for start, end in multi_ranges] - return [item for sublist in extracted_elements for item in sublist] + if not all(0 <= start < end <= len(sequence) for start, end in multi_ranges): + raise ValueError("Range indices must satisfy 0 ≤ start < end ≤ len(sequence)") + + return [item for start, end in multi_ranges for item in sequence[start:end]]
233-243: Replace magic indices with named constants for maintainability.Hard-coded offsets (
1:,30,0) obscure intent and are brittle if the landmark format changes.DRIVEN_FRAME_IDX = 1 NOISE_LANDMARK_IDX = 30 X_COORD_IDX = 0 "face_location": lmd_obj_full[DRIVEN_FRAME_IDX:, NOISE_LANDMARK_IDX, X_COORD_IDX],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/visualizr/dataset.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
src/visualizr/dataset.py
175-175: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
🪛 Pylint (3.3.7)
src/visualizr/dataset.py
[refactor] 14-14: Class 'LatentDataLoader' inherits from object, can be safely removed from bases in python3
(R0205)
[refactor] 14-14: Too many instance attributes (11/7)
(R0902)
[refactor] 15-15: Too many arguments (13/5)
(R0913)
[refactor] 15-15: Too many positional arguments (13/5)
(R0917)
[refactor] 15-15: Too many local variables (24/15)
(R0914)
[refactor] 49-49: Redefining argument with the local name 'db_name'
(R1704)
[refactor] 15-15: Too many statements (62/50)
(R0915)
[refactor] 201-201: Too many local variables (18/15)
(R0914)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Trunk Code Quality
- GitHub Check: Qodana
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Hi @MH0386! Your pull request is ready for merging |
|
Hi @MH0386, Your PR is in conflict and cannot be merged. |
…e project dictionary.
|
|
Thank you for your contribution @MH0386! Your pull request has been merged. |
1 participant
Summary by Sourcery
Clean up code comments and formatting across model modules, update CI configuration bootstrap steps, and remove obsolete configuration files
Enhancements:
CI:
Documentation:
Chores:
Summary by CodeRabbit
Chores
Style / Documentation
Refactor
Bug Fixes