Fix addressing #38 and #39#40
Conversation
📝 WalkthroughWalkthroughA new helper Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main.py`:
- Line 85: The run_prism signature was changed and no longer accepts a per-call
output path, breaking IHM callers that pass f"output_{ist}_{img}" and expect
artifacts in that folder; restore per-call output handling by adding an output
parameter to run_prism (e.g., def run_prism(coords, mass, radius, ps_names,
args, output=None)) and, at the start of run_prism, set args.output = output if
output is provided (falling back to the existing args.output when None) so
existing behavior remains backward compatible; ensure callers in ihm_parser.py
that currently pass a per-model folder continue to pass it and any other
invocations without an output argument still work.
- Around line 98-101: Replace the broken NumPy usage and len() calls by
computing an integer worker count safely: get system_cpus = os.cpu_count() or 1,
then compute worker_count = max(1, min(system_cpus - 1, args.cores)) (ensure
system_cpus-1 doesn't go below 1), use that integer in the chunksize calculation
(chunksize, extra = divmod(coords.shape[1], worker_count * 4)) and adjust the
extra-handling branch; refer to the existing variables cores_, chunksize,
coords.shape, and args.cores when making this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| cores_ = np.min(os.cpu_count() - 1, args.cores) | ||
| chunksize, extra = divmod(coords.shape[1], len(cores_) * 4) | ||
| if extra: | ||
| chunksize += 1 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the signature of numpy.min, and does np.min(x, y)compare two scalar values or treatyas theaxisparameter? Also, what happens in Python when callinglen() on an integer?
💡 Result:
The signature of numpy.min (np.min) is np.min(a, axis=None, out=None, keepdims=, initial=, where=True). It computes the minimum of the array elements. np.min(x, y) where x and y are scalars treats y as the axis parameter (likely resulting in an error if y is not a valid axis index, e.g., TypeError or IndexError), not as a second value to compare. To compare two scalars, use np.minimum(x, y) instead. Calling len on an integer in Python raises TypeError: object of type 'int' has no len. Integers are not iterable or sequence types that support len.
Citations:
- 1: https://numpy.org/doc/stable/reference/generated/numpy.min.html
- 2: https://numpy.org/doc/2.2/reference/generated/numpy.min.html
- 3: https://numpy.org/doc/1.25/reference/generated/numpy.min.html
- 4: https://www.datacamp.com/doc/numpy/min
- 5: https://numpy.org/doc/1.19/reference/generated/numpy.amin.html
- 6: https://numpy.org/doc/1.21/reference/generated/numpy.minimum.html
- 7: https://medium.com/@amit25173/understanding-numpy-minimum-with-examples-ac660d10296c
- 8: https://stackoverflow.com/questions/35204529/typeerror-object-of-type-int-has-no-len-error-assistance-needed
- 9: https://stackoverflow.com/questions/73825057/python-len-function-not-working-as-expected-and-giving-me-the-error-typeerro
- 10: https://medium.com/@abuolubunmi21/understanding-typeerror-using-len-with-integers-in-python-77546c4a6cc4
- 11: https://techgeekbuzz.com/blog/python-typeerror-object-of-type-int-has-no-len-solution
- 12: https://careerkarma.com/blog/python-typeerror-object-of-type-int-has-no-len/
🏁 Script executed:
cd /tmp && find . -name "main.py" -type f 2>/dev/null | head -5Repository: isblab/prism
Length of output: 38
🏁 Script executed:
git ls-files | grep -E "main\.py|src/" | head -20Repository: isblab/prism
Length of output: 342
🏁 Script executed:
wc -l src/main.pyRepository: isblab/prism
Length of output: 71
🏁 Script executed:
sed -n '95,105p' src/main.pyRepository: isblab/prism
Length of output: 361
🏁 Script executed:
grep -n "args.cores\|argparse\|add_argument.*cores" src/main.py | head -20Repository: isblab/prism
Length of output: 272
🏁 Script executed:
head -50 src/main.pyRepository: isblab/prism
Length of output: 1990
Fix the worker-count/chunk-size calculation before opening the pool.
np.min(os.cpu_count() - 1, args.cores) misuses the NumPy API—the second argument is treated as the axis parameter, not a value to compare. Line 99 then calls len() on the resulting scalar, which will raise TypeError: object of type 'int' has no len(). This code will fail before either Pool(...) starts, and it can also produce 0 workers on single-core or unknown-CPU hosts.
Suggested fix
- cores_ = np.min(os.cpu_count() - 1, args.cores)
- chunksize, extra = divmod(coords.shape[1], len(cores_) * 4)
- if extra:
- chunksize += 1
+ cpu_count = os.cpu_count() or 1
+ usable_cores = cpu_count - 1 if cpu_count > 1 else 1
+ cores_ = max(1, min(usable_cores, args.cores))
+ chunksize, extra = divmod(coords.shape[1], cores_ * 4)
+ if extra:
+ chunksize += 1
+ chunksize = max(1, chunksize)📝 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.
| cores_ = np.min(os.cpu_count() - 1, args.cores) | |
| chunksize, extra = divmod(coords.shape[1], len(cores_) * 4) | |
| if extra: | |
| chunksize += 1 | |
| cpu_count = os.cpu_count() or 1 | |
| usable_cores = cpu_count - 1 if cpu_count > 1 else 1 | |
| cores_ = max(1, min(usable_cores, args.cores)) | |
| chunksize, extra = divmod(coords.shape[1], cores_ * 4) | |
| if extra: | |
| chunksize += 1 | |
| chunksize = max(1, chunksize) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main.py` around lines 98 - 101, Replace the broken NumPy usage and len()
calls by computing an integer worker count safely: get system_cpus =
os.cpu_count() or 1, then compute worker_count = max(1, min(system_cpus - 1,
args.cores)) (ensure system_cpus-1 doesn't go below 1), use that integer in the
chunksize calculation (chunksize, extra = divmod(coords.shape[1], worker_count *
4)) and adjust the extra-handling branch; refer to the existing variables
cores_, chunksize, coords.shape, and args.cores when making this change.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main.py (1)
65-85:⚠️ Potential issue | 🔴 CriticalHarden worker/chunksize calculation and use the computed worker count.
Line 65 can still fail when
os.cpu_count()isNone, and on single-core hosts it can yield0, which makes Line 66 divide by zero. Also, Line 80 should use the computed worker count, notargs.cores.Proposed fix
- cores_ = min(os.cpu_count() - 1, args.cores) - chunksize, extra = divmod(coords.shape[1], cores_ * 4) + cpu_count = os.cpu_count() or 1 + usable_cores = max(1, cpu_count - 1) + cores_ = max(1, min(usable_cores, args.cores)) + chunksize, extra = divmod(coords.shape[1], cores_ * 4) if extra: chunksize += 1 + chunksize = max(1, chunksize) @@ - with Pool(args.cores) as p: + with Pool(cores_) as p:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.py` around lines 65 - 85, Protect against os.cpu_count() returning None and single-core hosts by computing workers = max(1, (os.cpu_count() or 1) - 1, 1) (or equivalent) and use that variable (e.g., cores_ or workers) everywhere; ensure the divmod denominator uses workers * 4 and never zero, compute chunksize with a safe fallback (e.g., if coords.shape[1] == 0 then chunksize = 1) and only increment chunksize when extra > 0, and finally pass the computed workers to Pool(...) instead of args.cores; update references to _part_func, chunksize, extra and the Pool(...) call to use the new workers variable consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main.py`:
- Line 145: The --return_spread argument is defined with action='store_true' but
default=True so it is always True; update the parser.add_argument call for
"--return_spread" to make it togglable by either setting default=False when
keeping action='store_true' or switching to argparse.BooleanOptionalAction to
provide paired --return-spread / --no-return-spread flags (referencing the
parser.add_argument invocation for "--return_spread" and the action parameter).
---
Duplicate comments:
In `@src/main.py`:
- Around line 65-85: Protect against os.cpu_count() returning None and
single-core hosts by computing workers = max(1, (os.cpu_count() or 1) - 1, 1)
(or equivalent) and use that variable (e.g., cores_ or workers) everywhere;
ensure the divmod denominator uses workers * 4 and never zero, compute chunksize
with a safe fallback (e.g., if coords.shape[1] == 0 then chunksize = 1) and only
increment chunksize when extra > 0, and finally pass the computed workers to
Pool(...) instead of args.cores; update references to _part_func, chunksize,
extra and the Pool(...) call to use the new workers variable consistently.
🪄 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: 7e10a549-ec36-447b-b4ed-b62334f48355
📒 Files selected for processing (2)
src/ihm_parser.pysrc/main.py
✅ Files skipped from review due to trivial changes (1)
- src/ihm_parser.py
| help="Type of input: npz/pdb/cif/rmf/dcd.", | ||
| required=True, type=str) | ||
| parser.add_argument("--voxel_size", "-v", help="Voxel size for density calculations", default=4, type=int) | ||
| parser.add_argument("--return_spread", "-rs", help="Return the spread bead_spread", action='store_true', default = True) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if src/main.py exists and examine the area around line 145
if [ -f src/main.py ]; then
echo "=== Checking src/main.py around line 145 ==="
wc -l src/main.py
sed -n '140,150p' src/main.py
else
echo "src/main.py not found at expected location"
# Try to find main.py
find . -name "main.py" -type f
fiRepository: isblab/prism
Length of output: 1113
🏁 Script executed:
# Search for any related --return-spread or --no-return-spread arguments
rg "return.spread|return_spread" --max-count 20Repository: isblab/prism
Length of output: 2391
🏁 Script executed:
# Look for other argparse action='store_true' patterns to understand the codebase's conventions
rg "action=.*store" -A 1 src/main.pyRepository: isblab/prism
Length of output: 269
--return_spread flag is non-toggleable and always evaluates to True.
With action='store_true' and default=True, the argument will be True whether the flag is provided or absent, making it impossible for users to disable this feature. Either change to default=False if toggling is desired, or use argparse.BooleanOptionalAction (Python 3.9+) with paired flags like --return-spread and --no-return-spread.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main.py` at line 145, The --return_spread argument is defined with
action='store_true' but default=True so it is always True; update the
parser.add_argument call for "--return_spread" to make it togglable by either
setting default=False when keeping action='store_true' or switching to
argparse.BooleanOptionalAction to provide paired --return-spread /
--no-return-spread flags (referencing the parser.add_argument invocation for
"--return_spread" and the action parameter).
shruthivis
left a comment
shruthivis
left a comment
There was a problem hiding this comment.
The diff has too many unrelated changes in too many unrelated lines.
Stick to 1 change in 1 PR. Submit a PR addressing only the imap issue with minimal code changes to make it work as expected.
Don't move around lines unless required (e.g. bead_spread=[] initialization).
2 participants
Addressing #38 ,output_dirargument fromrun_prismwas removedFor addressing #39 , the following changes were made
Summary by CodeRabbit
Refactor
Style