Skip to content

fix(log): pass and log altloc_occ dict rather than float to get base map#169

Merged
marcuscollins merged 2 commits intomainfrom
dm/issue-160
Mar 15, 2026
Merged

fix(log): pass and log altloc_occ dict rather than float to get base map#169
marcuscollins merged 2 commits intomainfrom
dm/issue-160

Conversation

@DorisMai
Copy link
Copy Markdown
Contributor

@DorisMai DorisMai commented Mar 14, 2026

Address issue #160

Summary by CodeRabbit

  • Refactor

    • Occupancy handling in grid-search evaluation updated to use explicit A/B occupancy mapping; logging updated to reflect the new mapping.
  • Tests

    • Added tests covering base-map path resolution for varied occupancy scenarios: existing/missing maps, zero-occupancy pruning, all-zero and over-sum occupancies triggering warnings.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 14, 2026

📝 Walkthrough

Walkthrough

Updates to base map path resolution in the grid-search script to pass altloc occupancies as a dict (A and B=1.0-A) and logging to match; tests extended to exercise ProteinConfig.get_base_map_path_for_occupancy with multiple occupancy scenarios and edge-case warnings.

Changes

Cohort / File(s) Summary
Grid search script
scripts/eval/rscc_grid_search_script.py
Constructs an altloc_occ dict { "A": occ, "B": 1.0 - occ }, passes it to get_base_map_path_for_occupancy, and updates logging to show the mapping instead of a scalar occupancy.
Tests for ProteinConfig
tests/eval/test_eval_dataclasses.py
Adds ProteinConfig imports, a protein_config pytest fixture, and new tests covering get_base_map_path_for_occupancy behavior for existing files, missing files, zero occupancies, and occupancies summing >1 with warnings.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • fix issue 17 #115 — Switches callers to pass altloc_occupancies dict and updates get_base_map_path_for_occupancy API.

Suggested reviewers

  • marcuscollins

Poem

🐰 In files where occupancies play,
A maps the lead, B steps away,
One minus the other, tidy and neat,
Tests hop in to keep paths complete. 🥕✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: passing and logging an altloc_occ dictionary instead of a float to the base map path function.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch dm/issue-160
📝 Coding Plan
  • Generate coding plan for human review comments

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@DorisMai DorisMai marked this pull request as ready for review March 14, 2026 17:45
@DorisMai DorisMai requested a review from k-chrispens March 14, 2026 17:45
for occ in args.occupancies:
path = config.get_base_map_path_for_occupancy(occ) # will warn if not found
altloc_occ = {"A": occ, "B": 1.0 - occ}
path = config.get_base_map_path_for_occupancy(altloc_occ) # will warn if not found
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this call actually work? (I'm just on a phone on the way to SFO so can't check rn) But I thought this method just took a float, not a dictionary?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe Karson has changed the related utils function to handle dict now. I can add a few tests.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/eval/test_eval_dataclasses.py`:
- Around line 136-174: The new fixture protein_config and the test methods in
class TestProteinConfigGetBaseMapPath (e.g., test_returns_path_when_file_exists,
test_returns_none_when_file_missing, test_zero_occ_altloc_dropped_from_filename,
test_all_zero_occupancies_returns_none_with_warning,
test_sum_exceeds_one_returns_none_with_warning) lack NumPy-style docstrings; add
concise NumPy-style docstrings to the protein_config fixture and each test
method describing purpose, parameters (if any), and return behavior (for the
fixture) following the repo's docstring pattern so linting passes.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 7dea3e41-203c-45da-ab4c-31d7c1b11f81

📥 Commits

Reviewing files that changed from the base of the PR and between db95a66 and 43c920f.

📒 Files selected for processing (1)
  • tests/eval/test_eval_dataclasses.py

Comment thread tests/eval/test_eval_dataclasses.py
Copy link
Copy Markdown
Collaborator

@marcuscollins marcuscollins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the extra tests! LGTM.

@marcuscollins marcuscollins merged commit 3871b0e into main Mar 15, 2026
3 of 4 checks passed
@marcuscollins
Copy link
Copy Markdown
Collaborator

Currently ignoring failing test in protenix-dev. This test passes when running locally.

@k-chrispens k-chrispens deleted the dm/issue-160 branch April 22, 2026 00:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants