Add additional type hints and update formatting#210
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughStandardized string quoting and formatting, added/expanded Python type annotations, switched some file I/O to context managers, changed one synthesis task lookup call, adjusted test subprocess handling and path computation, and made minor signature/annotation updates across docs, examples, core modules, tools, and tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
logik/tools/fasm_to_bitstream/bitstream_bin_convert.py (1)
21-30: Stream conversion directly to reduce peak memory usage.Current flow buffers every formatted byte in
wordsbefore writing. For large bitstreams, writing per-byte during read is simpler and more memory-efficient.♻️ Suggested refactor
- words = [] - with open(bin_filename, "rb") as bin_file: - for cur_byte in bin_file.read(): - formatted_word = format(cur_byte, "0x").zfill(2) - words.append(formatted_word) - - with open(out_filename, "w") as out_file: - for word in words: - out_file.write(word) - out_file.write("\n") + with open(bin_filename, "rb") as bin_file, open(out_filename, "w") as out_file: + for cur_byte in bin_file.read(): + out_file.write(format(cur_byte, "0x").zfill(2)) + out_file.write("\n")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@logik/tools/fasm_to_bitstream/bitstream_bin_convert.py` around lines 21 - 30, The current implementation builds a list `words` of all formatted bytes which wastes memory for large bitstreams; change `bitstream_bin_convert.py` to stream-convert by opening `bin_filename` and `out_filename` and writing each formatted byte as you read it (avoid accumulating into `words`), i.e., replace the loop that appends to `words` with a read-while loop that formats each `cur_byte` to `formatted_word` and immediately writes it plus newline to the output file; keep the same formatting logic and variable names (`cur_byte`, `formatted_word`) to locate and update the code.examples/adder/pin_constraints.py (1)
87-90: Redundantclose()call inside context manager.The
with open(...)context manager automatically closes the file when exiting the block. The explicitjson_file.close()on line 90 is unnecessary.♻️ Suggested fix
def write_json_constraints(constraints, filename): with open(filename, "w") as json_file: json_file.write(json.dumps(constraints, indent=2)) json_file.write("\n") - json_file.close()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/adder/pin_constraints.py` around lines 87 - 90, Inside the with open(filename, "w") as json_file: block you should remove the redundant json_file.close() call (the context manager closes the file automatically); keep the json_file.write(json.dumps(constraints, indent=2)) and the trailing json_file.write("\n") lines but delete the explicit close to avoid unnecessary/incorrect resource handling around json_file, filename, and constraints.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@logik/tools/fasm_to_bitstream/bitstream_finish.py`:
- Around line 34-39: There is an extra blank line in the run() method before the
fasm_file assignment causing flake8 E303; edit the run() function (around where
fpga = self.project.get("fpga", "device") and fpga_obj =
self.project.get("library", fpga, field="schema") are defined) to remove the
blank line so the fasm_file = f"inputs/{self.design_topmodule}.fasm" statement
immediately follows the fpga_obj assignment.
In `@logik/tools/fasm_to_bitstream/fasm_to_bitstream.py`:
- Around line 265-266: The code incorrectly compares the string variable
feature_value to integer 0, so strings like "0" are treated as enabled; change
the check before appending to canonical_fasm_feature_list to properly detect
zero values by converting feature_value to an integer (e.g., int(feature_value)
!= 0) with a safe fallback (try/except ValueError) or by comparing to the string
"0" (feature_value != "0") to ensure only non-zero scalar features are appended;
update the block that uses feature_name and feature_value accordingly.
---
Nitpick comments:
In `@examples/adder/pin_constraints.py`:
- Around line 87-90: Inside the with open(filename, "w") as json_file: block you
should remove the redundant json_file.close() call (the context manager closes
the file automatically); keep the json_file.write(json.dumps(constraints,
indent=2)) and the trailing json_file.write("\n") lines but delete the explicit
close to avoid unnecessary/incorrect resource handling around json_file,
filename, and constraints.
In `@logik/tools/fasm_to_bitstream/bitstream_bin_convert.py`:
- Around line 21-30: The current implementation builds a list `words` of all
formatted bytes which wastes memory for large bitstreams; change
`bitstream_bin_convert.py` to stream-convert by opening `bin_filename` and
`out_filename` and writing each formatted byte as you read it (avoid
accumulating into `words`), i.e., replace the loop that appends to `words` with
a read-while loop that formats each `cur_byte` to `formatted_word` and
immediately writes it plus newline to the output file; keep the same formatting
logic and variable names (`cur_byte`, `formatted_word`) to locate and update the
code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c17ad505-5e5c-4107-90a0-425efc25f2b0
📒 Files selected for processing (17)
docs/source/conf.pyexamples/adder/adder.pyexamples/adder/pin_constraints.pyexamples/eth_mac_1g/eth_mac_1g.pyexamples/picorv32/picorv32.pylogik/devices/logik_fpga.pylogik/flows/logik_flow.pylogik/tools/fasm_to_bitstream/bitstream_bin_convert.pylogik/tools/fasm_to_bitstream/bitstream_finish.pylogik/tools/fasm_to_bitstream/fasm_to_bitstream.pypyproject.tomltests/conftest.pytests/data/z1000.pytests/examples/test_adder.pytests/examples/test_eth_mac_1g.pytests/examples/test_files.pytests/examples/test_picorv32.py
Comment on lines
+265
to
266
| if feature_value != 0: | ||
| canonical_fasm_feature_list.append(feature_name) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the problematic condition exists and confirm Python comparison behavior.
rg -n -C2 'if feature_value != 0' logik/tools/fasm_to_bitstream/fasm_to_bitstream.py
python - <<'PY'
print('"0" != 0 ->', "0" != 0)
print('"1" != 0 ->', "1" != 0)
PYRepository: siliconcompiler/logik
Length of output: 258
🏁 Script executed:
# Read the full context around lines 265-266 to understand how feature_value is assigned
sed -n '250,280p' logik/tools/fasm_to_bitstream/fasm_to_bitstream.py | cat -nRepository: siliconcompiler/logik
Length of output: 1468
🏁 Script executed:
# Search for where feature_value is assigned in this function/block
sed -n '220,280p' logik/tools/fasm_to_bitstream/fasm_to_bitstream.py | cat -nRepository: siliconcompiler/logik
Length of output: 2878
Fix incorrect zero-value comparison for scalar FASM features.
At line 265, feature_value is a string from the split operation at line 226. The condition feature_value != 0 always evaluates to true in Python (since a string is never equal to an integer), causing entries like FEATURE=0 to be incorrectly treated as enabled and producing wrong bitstreams.
Fix
- if feature_value != 0:
+ if feature_value != "0":
canonical_fasm_feature_list.append(feature_name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@logik/tools/fasm_to_bitstream/fasm_to_bitstream.py` around lines 265 - 266,
The code incorrectly compares the string variable feature_value to integer 0, so
strings like "0" are treated as enabled; change the check before appending to
canonical_fasm_feature_list to properly detect zero values by converting
feature_value to an integer (e.g., int(feature_value) != 0) with a safe fallback
(try/except ValueError) or by comparing to the string "0" (feature_value != "0")
to ensure only non-zero scalar features are appended; update the block that uses
feature_name and feature_value accordingly.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
logik/tools/fasm_to_bitstream/fasm_to_bitstream.py (1)
10-14: Update type annotation forformat_binary_bitstreamreturn type to usenumpy.typing.NDArray.The function returns
np.array(converted_data, np.uint8)but is currently annotated asndarray[Any, dtype], which provides weak static typing. UseNDArray[np.uint8]for explicit, more type-safe annotation.♻️ Proposed typing refinement
-from typing import Any +from typing import Any import numpy as np -from numpy import dtype, ndarray +from numpy.typing import NDArray ... -def format_binary_bitstream(bitstream_data, word_size=8) -> ndarray[Any, dtype]: +def format_binary_bitstream( + bitstream_data, word_size=8 +) -> NDArray[np.uint8]:Also applies to: 176-176
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@logik/tools/fasm_to_bitstream/fasm_to_bitstream.py` around lines 10 - 14, Update the return type annotation of the function format_binary_bitstream from the generic ndarray[Any, dtype] to a precise numpy.typing.NDArray[np.uint8]; import NDArray from numpy.typing and use NDArray[np.uint8] as the function's return annotation (and update the same annotation instance elsewhere in this module, e.g., the other occurrence at the end of the file) so the returned np.array(..., np.uint8) is correctly typed.docs/source/conf.py (1)
15-15: Use a targetednoqadirective.Change
# noqa E402to# noqa: E402to properly suppress only the E402 violation. The colon syntax ensures the suppression targets the specific error code rather than acting as a blanket filter.Suggested change
-import logik # noqa E402 +import logik # noqa: E402🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/source/conf.py` at line 15, Update the import suppression to target only E402: in the line importing the module (import logik) replace the blanket comment "# noqa E402" with the targeted form "# noqa: E402" so the import statement in conf.py only suppresses that specific lint error for the import.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/adder/pin_constraints.py`:
- Around line 12-15: The help text in option_parser.add_argument advertises
"all" even though main() only supports "raw" or a single mapped part; either
remove "or specify all to build all parts in parts catalog" from the
add_argument help string, or add handling in main() to detect the "all" value
and iterate the parts catalog (the same data structure main currently uses to
map part names) to generate constraints for each part; update main() to call the
existing constraint-generation routine for every entry when "all" is passed and
ensure errors/logging behave as they do for single-part processing.
In `@logik/tools/fasm_to_bitstream/fasm_to_bitstream.py`:
- Around line 264-266: The code is comparing feature_value (a string from
feature_fields[1]) to integer 0, causing "0" != 0 to be true and disabled
features to be appended; update the check in the block that appends to
canonical_fasm_feature_list (using feature_name) to either convert feature_value
to int before comparison (e.g., int(feature_value) != 0) or compare against the
string "0" (feature_value != "0") so only enabled single-bit features are
included; ensure this change is applied where feature_fields, feature_value, and
canonical_fasm_feature_list are used.
In `@tests/conftest.py`:
- Around line 18-20: The setup function parameter named dir shadows the built-in
dir(), so rename the parameter in the setup function signature and all its
usages to example_dir to avoid Ruff A002; update the function def setup(dir) to
def setup(example_dir) and change the local usage ex_dir =
os.path.join(cad_root, "examples", dir) to use example_dir (and any other
references within setup) so behavior remains identical but the builtin is not
shadowed.
- Around line 35-37: The ebrick_fpga_cad_root fixture builds the repo root by
string-replacing forward slashes which breaks on Windows; change its
implementation to use os.path.dirname(os.path.abspath(__file__)) twice (i.e.,
call os.path.dirname on the file path and then again) so it computes the tests
directory parent portably instead of using .replace on "/conftest.py" and
"/tests". Ensure you update the function ebrick_fpga_cad_root to use these
os.path.dirname calls and remove the hard-coded replace logic.
---
Nitpick comments:
In `@docs/source/conf.py`:
- Line 15: Update the import suppression to target only E402: in the line
importing the module (import logik) replace the blanket comment "# noqa E402"
with the targeted form "# noqa: E402" so the import statement in conf.py only
suppresses that specific lint error for the import.
In `@logik/tools/fasm_to_bitstream/fasm_to_bitstream.py`:
- Around line 10-14: Update the return type annotation of the function
format_binary_bitstream from the generic ndarray[Any, dtype] to a precise
numpy.typing.NDArray[np.uint8]; import NDArray from numpy.typing and use
NDArray[np.uint8] as the function's return annotation (and update the same
annotation instance elsewhere in this module, e.g., the other occurrence at the
end of the file) so the returned np.array(..., np.uint8) is correctly typed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1d0dd0fe-87aa-422a-a21b-ab53089c4d41
📒 Files selected for processing (17)
docs/source/conf.pyexamples/adder/adder.pyexamples/adder/pin_constraints.pyexamples/eth_mac_1g/eth_mac_1g.pyexamples/picorv32/picorv32.pylogik/devices/logik_fpga.pylogik/flows/logik_flow.pylogik/tools/fasm_to_bitstream/bitstream_bin_convert.pylogik/tools/fasm_to_bitstream/bitstream_finish.pylogik/tools/fasm_to_bitstream/fasm_to_bitstream.pypyproject.tomltests/conftest.pytests/data/z1000.pytests/examples/test_adder.pytests/examples/test_eth_mac_1g.pytests/examples/test_files.pytests/examples/test_picorv32.py
Comment on lines
264
to
266
| else: | ||
| if (feature_value != 0): | ||
| if feature_value != 0: | ||
| canonical_fasm_feature_list.append(feature_name) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Python 3, does the expression "0" != 0 evaluate to True, and what is the correct way to compare a parsed string numeric value against zero?
💡 Result:
Yes, in Python 3, the expression "0" != 0 evaluates to True. The correct way to compare a parsed string numeric value (like a string containing digits that represents a number) against zero is to convert it to a numeric type first, then compare. Use int(value) != 0 or float(value) != 0 depending on whether integer or float precision is needed. Wrap in try-except to handle invalid numeric strings. Example: value = "0" try: num = int(value) if num != 0: print("Not zero") except ValueError: print("Not a valid integer") This follows Python's official documentation on comparisons: objects of different types (str vs int) compare unequal using !=, and numeric comparisons require compatible types.
Citations:
- 1: https://www.digitalocean.com/community/tutorials/python-not-equal-operator
- 2: https://docs.python.org/3.15/library/stdtypes.html
- 3: https://docs.python.org/3/reference/expressions.html?highlight=comparison
- 4: https://www.digitalocean.com/community/tutorials/python-string-comparison
- 5: https://stackoverflow.com/questions/33977659/how-do-i-compare-a-string-and-an-integer
- 6: https://docs.python.org/3/library/stdtypes.html
- 7: https://stackoverflow.com/questions/57236475/why-0-and-0-are-evaluated-to-false
- 8: https://docs.python.org/3/library/stdtypes
- 9: https://stackoverflow.com/questions/354038/how-do-i-check-if-a-string-represents-a-number-float-or-int
- 10: https://sqlpey.com/python/python-string-to-number-validation/
- 11: https://prosperouswaydown.com/how-to-check-string-equals-zero-in-python/
🏁 Script executed:
# First, locate and examine the file
find . -name "fasm_to_bitstream.py" -type fRepository: siliconcompiler/logik
Length of output: 117
🏁 Script executed:
# Examine the file around line 264-266 and surrounding context
sed -n '240,290p' ./logik/tools/fasm_to_bitstream/fasm_to_bitstream.py | cat -nRepository: siliconcompiler/logik
Length of output: 2397
🏁 Script executed:
# Look at more context to understand how feature_value is assigned
sed -n '200,280p' ./logik/tools/fasm_to_bitstream/fasm_to_bitstream.py | cat -nRepository: siliconcompiler/logik
Length of output: 3615
Fix string-vs-int comparison bug in single-bit feature filtering.
Line 266 compares string feature_value (from feature_fields[1]) to integer 0, causing "0" != 0 to evaluate to True. This incorrectly appends disabled features (where value equals "0") to the canonical feature list.
Fix
- if feature_value != 0:
+ if feature_value != "0":
canonical_fasm_feature_list.append(feature_name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@logik/tools/fasm_to_bitstream/fasm_to_bitstream.py` around lines 264 - 266,
The code is comparing feature_value (a string from feature_fields[1]) to integer
0, causing "0" != 0 to be true and disabled features to be appended; update the
check in the block that appends to canonical_fasm_feature_list (using
feature_name) to either convert feature_value to int before comparison (e.g.,
int(feature_value) != 0) or compare against the string "0" (feature_value !=
"0") so only enabled single-bit features are included; ensure this change is
applied where feature_fields, feature_value, and canonical_fasm_feature_list are
used.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
logik/tools/fasm_to_bitstream/fasm_to_bitstream.py (1)
239-240:⚠️ Potential issue | 🔴 CriticalFix scalar feature zero check (string vs int).
At Line 239,
feature_valueis a string. Comparing it to integer0makes"0"pass as non-zero and incorrectly enables disabled scalar features.🐛 Proposed fix
- if feature_value != 0: + if feature_value != "0": canonical_fasm_feature_list.append(feature_name)#!/bin/bash # Verify current condition and Python behavior. rg -n -C2 'if feature_value != 0' logik/tools/fasm_to_bitstream/fasm_to_bitstream.py python - <<'PY' print('"0" != 0 ->', "0" != 0) print('"1" != 0 ->', "1" != 0) PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@logik/tools/fasm_to_bitstream/fasm_to_bitstream.py` around lines 239 - 240, The check treating scalar feature_value as an int is wrong because feature_value is a string; update the condition in the block that appends to canonical_fasm_feature_list (reference: variable feature_value and list canonical_fasm_feature_list) to test the string value correctly—e.g., convert feature_value to an int safely (catching conversion errors) or compare against the string "0" so that "0" is treated as disabled and only non-zero values append the feature_name; ensure any conversion preserves existing behavior for non-numeric values.
🧹 Nitpick comments (1)
logik/tools/fasm_to_bitstream/fasm_to_bitstream.py (1)
151-151: Usenumpy.typing.NDArray[np.uint8]for precise return typing.The function at line 151 always returns a
np.uint8array (as seen in line 163:np.array(converted_data, np.uint8)), so the currentndarray[Any, dtype]annotation is unnecessarily broad and can trigger noise under strict type checking. Update the import fromfrom numpy import dtype, ndarraytofrom numpy.typing import NDArrayand change the return type toNDArray[np.uint8].♻️ Proposed typing refinement
-from numpy import dtype, ndarray +from numpy.typing import NDArray -def format_binary_bitstream(bitstream_data, word_size=8) -> ndarray[Any, dtype]: +def format_binary_bitstream(bitstream_data, word_size=8) -> NDArray[np.uint8]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@logik/tools/fasm_to_bitstream/fasm_to_bitstream.py` at line 151, The return type of format_binary_bitstream should be narrowed to NDArray[np.uint8]; update the imports by replacing "from numpy import dtype, ndarray" with "from numpy.typing import NDArray" and change the function signature to return NDArray[np.uint8] (keeping the existing implementation that constructs np.array(converted_data, np.uint8)); ensure any other references to ndarray/dtype in this module are updated or left intact as appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@logik/tools/fasm_to_bitstream/fasm_to_bitstream.py`:
- Around line 239-240: The check treating scalar feature_value as an int is
wrong because feature_value is a string; update the condition in the block that
appends to canonical_fasm_feature_list (reference: variable feature_value and
list canonical_fasm_feature_list) to test the string value correctly—e.g.,
convert feature_value to an int safely (catching conversion errors) or compare
against the string "0" so that "0" is treated as disabled and only non-zero
values append the feature_name; ensure any conversion preserves existing
behavior for non-numeric values.
---
Nitpick comments:
In `@logik/tools/fasm_to_bitstream/fasm_to_bitstream.py`:
- Line 151: The return type of format_binary_bitstream should be narrowed to
NDArray[np.uint8]; update the imports by replacing "from numpy import dtype,
ndarray" with "from numpy.typing import NDArray" and change the function
signature to return NDArray[np.uint8] (keeping the existing implementation that
constructs np.array(converted_data, np.uint8)); ensure any other references to
ndarray/dtype in this module are updated or left intact as appropriate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f3ba0156-d79c-4dfb-bcb1-bcb74e35d8c0
📒 Files selected for processing (2)
docs/source/conf.pylogik/tools/fasm_to_bitstream/fasm_to_bitstream.py
petergrossmann21
approved these changes
Apr 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Code Quality & Maintenance
Documentation
Testing & Infrastructure