README.md updated with instructions to run code, cleaning up toml file#45
Conversation
marcuscollins
left a comment
marcuscollins
left a comment
There was a problem hiding this comment.
I think this still needs instructions on how to prepare the data. Please put that in the section "Data Preparation", I think that's where readers will expect it to be.
Small PR that adds graph creation constants to `constants.py` and makes changes to `utils.py` to use those constants (as well slightly improving the tqdm logging utility function. Also makes formatting changes to util plotting functions. Style changes (organizing import order) should be addressed after merging PR #39 and #45 where pre-commit hooks run to check and fix these things (address issue #27 ).
…cks pymol-open-source install
968e219 to
e233a94
Compare
DorisMai
left a comment
DorisMai
left a comment
There was a problem hiding this comment.
README will likely needs update soon.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
There was a problem hiding this comment.
Pull request overview
This PR updates developer-facing setup/run documentation and streamlines the project’s dependency configuration (including regenerating uv.lock) to better reflect the intended “direct dependencies only” approach.
Changes:
- Simplifies
pyproject.tomldependencies and dev dependency group; keeps customuvindexes/sources for PyTorch + PyG. - Regenerates
uv.lockto reflect the cleaned dependency set and updated resolutions. - Expands
README.mdwith detailed project structure, data/cache expectations, and run commands; updates.gitignoreto ignorefigures/.
Reviewed changes
Copilot reviewed 2 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
uv.lock |
Updated lockfile after dependency cleanup and re-resolution. |
pyproject.toml |
Pruned dependencies/dev-dependencies; kept uv index configuration and lint/type tool configs. |
README.md |
Major expansion of setup + usage documentation and expected directory/cache structures. |
.gitignore |
Adds figures/ to ignored paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
133-145:⚠️ Potential issue | 🟠 MajorKeep the fallback install path aligned with
pyproject.toml.The manual
uv pip install ...list is missing 8 runtime packages actively used in the codebase:esm,loguru,tqdm,pandas,scipy,matplotlib,pillow, andpymol-open-source. Users following this branch will not have a complete environment matchinguv sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 133 - 145, Update the README manual install command block so the fallback path matches pyproject.toml by adding the missing runtime packages: esm, loguru, tqdm, pandas, scipy, matplotlib, pillow, and pymol-open-source to the `uv pip install ...` list (the bash block shown under "Installing the environment"); ensure the installed package list mirrors what `uv sync`/pyproject.toml declares so users get an equivalent environment when using the manual commands.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pyproject.toml`:
- Around line 5-22: Add numpy as a direct runtime dependency in the project's
dependency list: modify the dependencies array in pyproject.toml (the section
currently containing "torch==2.8.0", "torch-geometric", etc.) to include "numpy"
so that modules like src/dataset.py, src/flow.py, and src/utils.py have an
explicit requirement; after updating pyproject.toml, regenerate your lockfile
(if any) or run the dependency install to ensure the change is applied.
In `@README.md`:
- Line 7: README contains unlabeled fenced code blocks that markdownlint flags;
update each affected fenced block (e.g., the tree and path examples referenced
around the unnamed fences and those at the noted locations) to include a
language label such as "text" (replace ``` with ```text) so markdownlint
recognizes them; locate the unnamed triple-backtick blocks in README.md
(including the ones near the tree/path examples and the reported positions) and
add the "text" label to each opening fence.
- Line 237: The README shows a workstation-specific default for the `--save_dir`
flag (`/home/srivasv/flow_checkpoints`); update the documentation to use a
portable default (e.g. `~/.flow_checkpoints` or `/var/lib/flow_checkpoints`) or
explicitly label the current value as an example, and make the same change for
the other occurrence referenced (line 248) so no machine-specific path appears
as a CLI default.
---
Outside diff comments:
In `@README.md`:
- Around line 133-145: Update the README manual install command block so the
fallback path matches pyproject.toml by adding the missing runtime packages:
esm, loguru, tqdm, pandas, scipy, matplotlib, pillow, and pymol-open-source to
the `uv pip install ...` list (the bash block shown under "Installing the
environment"); ensure the installed package list mirrors what `uv
sync`/pyproject.toml declares so users get an equivalent environment when using
the manual commands.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2e89bcaf-a79c-4df8-be39-24c755fd2b1a
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
.gitignoreREADME.mdpyproject.toml
| ## Project Structure | ||
|
|
||
| We use `uv` for our environment and package management, with python 3.12. | ||
| ``` |
There was a problem hiding this comment.
Add languages to these fenced blocks.
markdownlint is flagging these unlabeled fences. text is enough for the tree and path examples here.
Also applies to: 45-45, 91-91, 102-102, 316-316
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 7, README contains unlabeled fenced code blocks that
markdownlint flags; update each affected fenced block (e.g., the tree and path
examples referenced around the unnamed fences and those at the noted locations)
to include a language label such as "text" (replace ``` with ```text) so
markdownlint recognizes them; locate the unnamed triple-backtick blocks in
README.md (including the ones near the tree/path examples and the reported
positions) and add the "text" label to each opening fence.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
137-145:⚠️ Potential issue | 🟡 MinorManual install instructions don't match pyproject.toml dependencies.
Line 144 installs
Bioandnetworkx, but these are not inpyproject.toml:
networkxwas removed from dependencies (per AI summary)Bio(biopython) is not listed in the current dependenciesAlso missing from the manual install:
scipy,pandas,numpy,matplotlib,pillow,loguru,tqdm,esm,pymol-open-sourcewhich are now inpyproject.toml.Consider updating to match
pyproject.tomlor recommenduv syncas the primary install method and remove the manual install section.📝 Suggested update
-uv pip install biotite wandb Bio networkx e3nn pytest pytest-cov +uv pip install biotite wandb e3nn esm pymol-open-source scipy pandas numpy matplotlib pillow loguru tqdmOr simply direct users to use
uv syncinstead of maintaining duplicate install instructions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 137 - 145, Update the manual install block in README.md so it either matches pyproject.toml or is removed in favor of recommending the project-managed install (use "uv sync"); specifically replace the current pip lines (the block containing "uv pip install ... Bio networkx ...") with either (a) a pip install list that includes all packages present in pyproject.toml (scipy, pandas, numpy, matplotlib, pillow, loguru, tqdm, esm, pymol-open-source, torch, torch_geometric, torch_cluster, torch_scatter, pyg_lib, biotite, wandb, pytest, pytest-cov) or (b) a single sentence instructing users to run "uv sync" (and optionally "uv venv ... && source ...") so the dependency source of truth is pyproject.toml; ensure the README no longer lists removed packages (e.g., networkx) unless they are present in pyproject.toml.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 98-126: The README's "Cache Directory Structure" is incorrect; the
code stores cached .pt files directly under processed_dir (see cache_path in
src/dataset.py) and embeds SLAE/ESM data as fields inside those .pt files (e.g.,
protein_slae_embedding, residue_embeddings) rather than separate
geometry/esm/slae subdirectories, and the preprocessing failures log is written
to <processed_dir>/preprocessing_failures.log; update the README to describe
that cached files live at <processed_dir>/<cache_key>.pt, list the internal
fields (geometry tensors, protein_slae_embedding, residue_embeddings,
atom37_coords, sequence, num_residues, etc.), and correct the failure-log
location to <processed_dir>/preprocessing_failures.log (alternatively, if you
prefer the subdirectory layout, implement directory creation and adjust
cache_path and embedding writers to save into geometry/, esm/, slae/ and move
the failure log accordingly).
---
Outside diff comments:
In `@README.md`:
- Around line 137-145: Update the manual install block in README.md so it either
matches pyproject.toml or is removed in favor of recommending the
project-managed install (use "uv sync"); specifically replace the current pip
lines (the block containing "uv pip install ... Bio networkx ...") with either
(a) a pip install list that includes all packages present in pyproject.toml
(scipy, pandas, numpy, matplotlib, pillow, loguru, tqdm, esm, pymol-open-source,
torch, torch_geometric, torch_cluster, torch_scatter, pyg_lib, biotite, wandb,
pytest, pytest-cov) or (b) a single sentence instructing users to run "uv sync"
(and optionally "uv venv ... && source ...") so the dependency source of truth
is pyproject.toml; ensure the README no longer lists removed packages (e.g.,
networkx) unless they are present in pyproject.toml.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
135-142:⚠️ Potential issue | 🟠 MajorManual installation command is out of sync with pyproject.toml.
The README's manual installation command lists packages that don't exist in the updated
pyproject.toml:Bioandnetworkx(line 141). The currentpyproject.tomlshows these were correctly removed from dependencies, but the README still references them.Additionally, the manual installation steps are incomplete—they omit dependencies present in
pyproject.tomllikepymol-open-source,scipy,pandas,numpy,matplotlib,pillow,loguru, andtqdm. Update line 141 to either remove the non-existent packages or include all main dependencies, or recommend users runuv syncinstead for a complete and correct installation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 135 - 142, The README's manual install commands are out of sync with pyproject.toml: remove obsolete packages (Bio, networkx) from the uv pip install line and either add the missing main dependencies (pymol-open-source, scipy, pandas, numpy, matplotlib, pillow, loguru, tqdm) to that manual command or replace the manual instruction with a single recommendation to run "uv sync" to install dependencies as defined in pyproject.toml; update the README entry around the virtualenv/pip commands and the specific uv pip line to reflect whichever option you choose and ensure package names match the pyproject.toml exactly.
♻️ Duplicate comments (1)
README.md (1)
122-122:⚠️ Potential issue | 🟡 MinorClarify the path reference for the preprocessing failures log.
The reference to
<geometry_dir>/preprocessing_failures.logis ambiguous given the three-layer cache structure. Based on the structure documented above (lines 100-101), this should likely be<processed_dir>/geometry/preprocessing_failures.logto be more explicit.📝 Proposed fix
-- Preprocessing failures are logged to `<geometry_dir>/preprocessing_failures.log` +- Preprocessing failures are logged to `<processed_dir>/geometry/preprocessing_failures.log`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 122, Update the README path reference: replace the ambiguous `<geometry_dir>/preprocessing_failures.log` string with the explicit `<processed_dir>/geometry/preprocessing_failures.log` to reflect the three-layer cache layout; edit the README.md occurrence of that path so it matches the documented structure (`<processed_dir>/geometry/preprocessing_failures.log`) everywhere it appears.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 273-280: Add documentation for configuring the EDIA output
directory by specifying the configuration option name (e.g., a command-line flag
like --edia_dir or a config key edia_dir), its default value, and when it is
required (only needed if EDIA filtering/processing is enabled). Update the EDIA
details block (the <edia_dir> reference) to show an example path and note the
default and override mechanism, and also add an entry to the "Key Training
Arguments" table listing the flag/key, description, default, and whether it is
optional or required when EDIA is enabled.
---
Outside diff comments:
In `@README.md`:
- Around line 135-142: The README's manual install commands are out of sync with
pyproject.toml: remove obsolete packages (Bio, networkx) from the uv pip install
line and either add the missing main dependencies (pymol-open-source, scipy,
pandas, numpy, matplotlib, pillow, loguru, tqdm) to that manual command or
replace the manual instruction with a single recommendation to run "uv sync" to
install dependencies as defined in pyproject.toml; update the README entry
around the virtualenv/pip commands and the specific uv pip line to reflect
whichever option you choose and ensure package names match the pyproject.toml
exactly.
---
Duplicate comments:
In `@README.md`:
- Line 122: Update the README path reference: replace the ambiguous
`<geometry_dir>/preprocessing_failures.log` string with the explicit
`<processed_dir>/geometry/preprocessing_failures.log` to reflect the three-layer
cache layout; edit the README.md occurrence of that path so it matches the
documented structure (`<processed_dir>/geometry/preprocessing_failures.log`)
everywhere it appears.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 4 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
pyproject.toml:46
esmpulls intorchvision(andtorchtext) as dependencies (seeuv.lock), buttool.uv.sourcesno longer pinstorchvisionto the CUDA 12.6 PyTorch index. This can lead to mixing CUDAtorchwheels fromdownload.pytorch.orgwith CPU wheels from PyPI for torchvision/torchtext, which is a common source of runtime import/ABI errors. Consider addingtorchvision(and likelytorchtext) back under[tool.uv.sources]pointing atpytorch-cu126, or otherwise ensuring these packages are resolved from a compatible PyTorch wheel index.
[tool.uv.sources]
torch = { index = "pytorch-cu126" }
torch-scatter = { index = "pyg-cu126" }
torch-cluster = { index = "pyg-cu126" }
pyg-lib = { index = "pyg-cu126" }
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| │ - protein_pos: centered protein coordinates (N, 3) | ||
| │ - protein_x: element one-hot encoding (N, 16) | ||
| │ - protein_res_idx: residue indices for grouping | ||
| │ - water_pos, water_x: water coordinates and features | ||
| │ - mate_pos, mate_x, mate_res_idx: symmetry mate data (if included) |
There was a problem hiding this comment.
The geometry cache description claims mate_pos, mate_x, and mate_res_idx are stored separately when include_mates=True, but ProteinWaterDataset currently concatenates mate atoms into protein_pos/protein_x and only stores num_asu_protein as metadata (no separate mate_* tensors are saved). Please update this section to reflect the actual cache contents (and, if needed, document how to recover the ASU vs mate boundary using num_asu_protein).
| │ - protein_pos: centered protein coordinates (N, 3) | |
| │ - protein_x: element one-hot encoding (N, 16) | |
| │ - protein_res_idx: residue indices for grouping | |
| │ - water_pos, water_x: water coordinates and features | |
| │ - mate_pos, mate_x, mate_res_idx: symmetry mate data (if included) | |
| │ - protein_pos: centered protein coordinates (N_protein, 3) | |
| │ - when include_mates=True, contains ASU atoms first, followed by symmetry mate atoms | |
| │ - protein_x: element one-hot encoding (N_protein, 16), aligned with protein_pos | |
| │ - protein_res_idx: residue indices for grouping, aligned with protein_pos | |
| │ - num_asu_protein: number of ASU protein atoms; mates start at index num_asu_protein (if present) | |
| │ - water_pos, water_x: water coordinates and features |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
README.md (1)
7-7:⚠️ Potential issue | 🟡 MinorAdd language specifiers to fenced code blocks.
Multiple fenced code blocks lack language specifiers, which
markdownlintflags and can affect rendering in some markdown viewers. Addtextas the language identifier for directory structure and file content examples.📝 Proposed fix
Line 7:
-``` +```text WaterFlow/Line 42:
-``` +```text <base_pdb_dir>/Line 88:
-``` +```text # Example: splits/train_list_0.95.txtLine 99:
-``` +```text <processed_dir>/Line 326:
-``` +```text <output_dir>/<run_name>/Also applies to: 42-42, 88-88, 99-99, 326-326
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 7, Several fenced code blocks in README.md are missing language specifiers; update each code fence that contains the directory/file examples to use the "text" language identifier. Specifically, change the opening fences for the blocks containing "WaterFlow/", "<base_pdb_dir>/", "# Example: splits/train_list_0.95.txt", "<processed_dir>/", and "<output_dir>/<run_name>/" to ```text so markdownlint stops flagging them and rendering is consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@README.md`:
- Line 7: Several fenced code blocks in README.md are missing language
specifiers; update each code fence that contains the directory/file examples to
use the "text" language identifier. Specifically, change the opening fences for
the blocks containing "WaterFlow/", "<base_pdb_dir>/", "# Example:
splits/train_list_0.95.txt", "<processed_dir>/", and "<output_dir>/<run_name>/"
to ```text so markdownlint stops flagging them and rendering is consistent.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pyproject.toml (1)
49-54: Consider removing unused mypy configuration or adding mypy to dev deps.The
[tool.mypy]configuration is present, butmypyis not listed in the dev dependencies (line 27). If the project has migrated totyfor type checking, consider removing the mypy config to avoid confusion. Otherwise, addmypyto dev dependencies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` around lines 49 - 54, The pyproject.toml contains a [tool.mypy] section but mypy is not listed in dev dependencies; either remove the [tool.mypy] configuration if you’ve standardized on a different type checker (e.g., ty) or add mypy to the dev dependencies so the config is honored. Edit the pyproject.toml to either delete the [tool.mypy] block or add "mypy" to the dev-dependencies table (matching existing dev deps format) so the project and config remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pyproject.toml`:
- Around line 49-54: The pyproject.toml contains a [tool.mypy] section but mypy
is not listed in dev dependencies; either remove the [tool.mypy] configuration
if you’ve standardized on a different type checker (e.g., ty) or add mypy to the
dev dependencies so the config is honored. Edit the pyproject.toml to either
delete the [tool.mypy] block or add "mypy" to the dev-dependencies table
(matching existing dev deps format) so the project and config remain consistent.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 4 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
pyproject.toml:47
esmpulls intorchvisiontransitively, buttorchvisionis no longer pinned to the PyTorch CUDA index. In the lockfile it resolves from PyPI (no+cu126), whiletorchis2.8.0+cu126. This can lead to ABI/extension mismatches at import/runtime. Consider restoringtorchvision(and likelytorchtext, also a compiled extension) in[tool.uv.sources]topytorch-cu126, then re-lock.
[tool.uv.sources]
torch = { index = "pytorch-cu126" }
torch-scatter = { index = "pyg-cu126" }
torch-cluster = { index = "pyg-cu126" }
pyg-lib = { index = "pyg-cu126" }
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| uv pip install torch==2.8.0 | ||
| uv pip install torch_geometric | ||
| uv pip install torch_cluster torch_scatter pyg_lib -f https://data.pyg.org/whl/torch-2.8.0+cu126.html | ||
| uv pip install biotite wandb Bio networkx e3nn pytest pytest-cov | ||
| uv pip install esm biotite pymol-open-source scipy pandas numpy matplotlib pillow loguru tqdm wandb e3nn | ||
| uv pip install pytest pytest-cov # dev dependencies |
There was a problem hiding this comment.
The manual install snippet installs torch==2.8.0 without specifying the CUDA 12.6 index. On Linux this is likely to pull the default (CPU) wheel from PyPI, which then won't match the later torch_cluster/torch_scatter/pyg_lib +cu126 wheels. Suggest either (a) documenting the uv index config / --index-url https://download.pytorch.org/whl/cu126 for the torch install step, or (b) explicitly recommending uv sync only and removing the manual pip instructions to avoid mismatched binaries.
|
|
||
| **PDB Parsing** | ||
| - Uses Biotite to extract protein atoms and water molecules (HOH residues) | ||
| - Modified residues are mapped to standard amino acids (MSE→M, SEC→U) |
There was a problem hiding this comment.
The “Modified residues are mapped to standard amino acids (MSE→M, SEC→U)” bullet reads like it happens during general PDB parsing, but the dataset preprocessing keeps modified residues (see parse_asu_with_biotite comment) and the mapping actually happens in the ESM embedding generation script. Consider clarifying that this mapping is applied when generating ESM embeddings (or when preparing sequences for encoders), not universally during geometry preprocessing.
| - Modified residues are mapped to standard amino acids (MSE→M, SEC→U) | |
| - Modified residues are retained during geometry preprocessing; common modified residues (e.g., MSE→M, SEC→U) are only mapped to standard amino acids when preparing sequences for encoders (e.g., in `scripts/generate_esm_embeddings.py`) |
There was a problem hiding this comment.
This looks like it is worth addressing.
marcuscollins
left a comment
marcuscollins
left a comment
There was a problem hiding this comment.
A few more little things to clean up, please take care of those before you merge but otherwise looks good.
|
|
||
| **PDB Parsing** | ||
| - Uses Biotite to extract protein atoms and water molecules (HOH residues) | ||
| - Modified residues are mapped to standard amino acids (MSE→M, SEC→U) |
There was a problem hiding this comment.
This looks like it is worth addressing.
| **PDB Parsing** | ||
| - Uses Biotite to extract protein atoms and water molecules (HOH residues) | ||
| - Modified residues are mapped to standard amino acids (MSE→M, SEC→U) | ||
| - Hydrogen atoms are excluded; only the first model is used |
There was a problem hiding this comment.
These should be separate bullet points. The second is especially important and I wouldn't want people to read over it.
| - Uses Biotite to extract protein atoms and water molecules (HOH residues) | ||
| - Modified residues are mapped to standard amino acids (MSE→M, SEC→U) | ||
| - Hydrogen atoms are excluded; only the first model is used | ||
| - For atoms with alternate conformations, the highest-occupancy conformer is selected |
There was a problem hiding this comment.
Is this something you ensure in your code, or is it a default behavior of Biotite? Note that here, and maybe add an issue to check whether we can guard against changes that might occur in that default behavior in the future.
There was a problem hiding this comment.
These are things I ensure in my code for uniformity -- to select model 1 and and set the alt conf flag to highest occupancy.
Summary by CodeRabbit
Documentation
Chores