Update tutorial1 #36
Conversation
WalkthroughAdds a new utility to generate ReST docs from dpnegf.utils.argcheck, updates docs structure and a tutorial notebook (text, logging, CLI steps, cleared outputs), and appends a commented Sphinx setup hook in docs/conf.py that has no runtime effect. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant Gen as gen_input_docs.main()
participant AC as argcheck (common_options/run_options)
participant R as render_argument()
participant FS as FileSystem
Dev->>Gen: invoke main()
Gen->>AC: call common_options() / run_options()
AC-->>Gen: return argument trees
loop per argument tree
Gen->>R: render_argument(tree)
R-->>Gen: RST text
Gen->>FS: write docs/input_params/<name>.rst
FS-->>Gen: ok
end
Gen-->>Dev: done
note right of R: recursive handling of sub_fields/sub_variants
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 1
🧹 Nitpick comments (16)
docs/index.rst (2)
13-17: Heading level looks consistent; minor polish optional."Overview" uses overline+underline; "Key Features:" uses underline only with "~". This renders, but consider aligning heading styles for consistency across the page.
41-42: Verify generated docs path exists in the tree.The toctree points to "input_params/index". Ensure your generator also creates this index.rst (or that it already exists), otherwise Sphinx will warn on missing documents.
docs/hands_on/tutorial1_c_chain.ipynb (8)
223-227: Close file handles when loading JSON.Use a context manager to avoid leaking file descriptors.
Apply:
- model_json = json.load(open(model)) + with open(model) as f: + model_json = json.load(f)
350-352: Use Python APIs instead of shell rm.Shell rm is OS‑specific and risks path issues. Prefer shutil.rmtree.
-if os.path.exists(results_path): - os.system('rm -r %s' % results_path) +import shutil +shutil.rmtree(results_path, ignore_errors=True)
568-579: Duplicate output directory handling.
outputis removed/created twice. Keep a single, idempotent setup block.- if os.path.exists(output): - os.system('rm -r %s' % output) - os.makedirs(output) ... -if os.path.exists(output): - os.system('rm -r %s' % output) -os.makedirs(output) +import shutil, os +shutil.rmtree(output, ignore_errors=True) +os.makedirs(output, exist_ok=True)
602-606: Guard the CLI invocation and be cross‑platform.
! dpnegfassumes PATH and POSIX tools. Add a check and fall back topython -m dpnegf.-! [ -d "../negf_output_cli" ] && rm -r ../negf_output_cli -! dpnegf run negf_chain_new.json -i nnsk_C_new.json -stu chain.vasp -o ../negf_output_cli +import shutil, sys, shutil as _sh +shutil.rmtree("../negf_output_cli", ignore_errors=True) +cmd = "dpnegf" if _sh.which("dpnegf") else f"{sys.executable} -m dpnegf" +! {cmd} run negf_chain_new.json -i nnsk_C_new.json -stu chain.vasp -o ../negf_output_cli
27-31: Fix typo: transmission.-2. how to calculate the tranmission spectrum +2. how to calculate the transmission spectrum
234-237: Fix typo: plotted.-After the model is loaded, bands for specific structures can be ploted. +After the model is loaded, bands for specific structures can be plotted.
222-223: Fix typo: demonstration.-model = "nnsk_C_new.json" # the model for demonsration +model = "nnsk_C_new.json" # the model for demonstration
33-35: Grammar: Requirements (plural).-### Requirement +### Requirementsdocs/conf.py (1)
97-101: If enabling the auto‑gen hook, resolve paths from docs/ and gate behind an env flag.Running from docs/ would write into docs/docs/input_params. Prefer resolving output_dir relative to this file and gate execution via an environment variable to avoid surprising RTD builds.
-# def setup(app): -# from dpnegf.utils import gen_input_docs -# gen_input_docs.main() +# def setup(app): +# # Only generate when explicitly enabled +# import os +# if os.getenv("DPNEGF_GEN_INPUT_DOCS") == "1": +# from pathlib import Path +# from dpnegf.utils import gen_input_docs +# out = Path(__file__).resolve().parent / "input_params" +# gen_input_docs.generate_rst_from_argcheck(output_dir=str(out))dpnegf/utils/gen_input_docs.py (5)
49-63: Generate an index.rst with a toctree.docs/index.rst expects input_params/index. Create it alongside generated pages.
for name, func in modules.items(): @@ with open(out_dir / f"{name}.rst", "w", encoding="utf-8") as f: f.write(rst) + + # write index.rst + toc = "\n".join(f" {n}" for n in modules.keys()) + index = f"""Input Parameters +=============== + +.. toctree:: + :maxdepth: 1 + +{toc} +""" + (out_dir / "index.rst").write_text(index, encoding="utf-8")
10-16: Format dtype lists more readably.If dtype is a list/tuple, render as a union string for RST.
- out.append(f"{ind}{arg.name}:") - out.append(f"{ind} | type: ``{arg.dtype}``") + out.append(f"{ind}{arg.name}:") + dtype = getattr(arg, "dtype", None) + if isinstance(dtype, (list, tuple)): + dtype_str = " | ".join(f"{t}" for t in dtype) + else: + dtype_str = f"{dtype}" + out.append(f"{ind} | type: ``{dtype_str}``")
18-22: Preserve bullet indentation in docs.Using
line.strip()removes intended indentation in multi-line docs. Keep leading spaces.- doc_lines = arg.doc.strip().splitlines() + doc_lines = arg.doc.strip("\n").splitlines() for line in doc_lines: - out.append(f"{ind} {line.strip()}") + out.append(f"{ind} {line.rstrip()}")
43-47: Replace full‑width punctuation in comments.Ruff RUF003: ambiguous
，. Use ASCII commas.
65-69: Support CLI usage.Allow specifying an output dir via CLI for local runs.
-if __name__ == "__main__": - main() +if __name__ == "__main__": + import argparse + p = argparse.ArgumentParser() + p.add_argument("-o", "--output-dir", default=None) + args = p.parse_args() + generate_rst_from_argcheck(args.output_dir)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/conf.py(1 hunks)docs/hands_on/tutorial1_c_chain.ipynb(4 hunks)docs/index.rst(1 hunks)dpnegf/utils/gen_input_docs.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
dpnegf/utils/gen_input_docs.py (1)
dpnegf/utils/argcheck.py (2)
common_options(71-93)run_options(1295-1323)
🪛 Ruff (0.12.2)
dpnegf/utils/gen_input_docs.py
43-43: Comment contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: run-tests
🔇 Additional comments (1)
docs/hands_on/tutorial1_c_chain.ipynb (1)
1-731: Strip outputs and reset execution_count in docs/hands_on/tutorial1_c_chain.ipynb — automated attempt failed (jupyter not found)The notebook still contains stdout/stderr/figure outputs and several non-null execution_count fields. My attempt to run:
/bin/bash: jupyter: command not found
so I could not clear outputs in this environment. Run one of the commands below locally or in CI and commit the cleared notebook:jupyter nbconvert --ClearOutputPreprocessor.enabled=True --inplace docs/hands_on/tutorial1_c_chain.ipynb
or
python - <<'PY'
import nbformat
nb = nbformat.read('docs/hands_on/tutorial1_c_chain.ipynb', as_version=4)
for cell in nb['cells']:
cell['outputs'] = []
cell['execution_count'] = None
nbformat.write(nb, 'docs/hands_on/tutorial1_c_chain.ipynb')
PYAfter clearing outputs and setting all execution_count to null, re-run verification.
| def generate_rst_from_argcheck(output_dir="docs/input_params"): | ||
| os.makedirs(output_dir, exist_ok=True) | ||
|
|
||
| # 这里你可以写死，也可以动态扫描 | ||
| modules = { | ||
| "common_options": argcheck.common_options, | ||
| "run_options": argcheck.run_options, | ||
| } |
There was a problem hiding this comment.
Default output_dir will be wrong when called from docs/conf.py.
Using "docs/input_params" relative to CWD creates docs/docs/input_params when invoked from docs/. Resolve relative to this file (repo root) or accept an explicit path.
-import os
+import os
+from pathlib import Path
...
-def generate_rst_from_argcheck(output_dir="docs/input_params"):
- os.makedirs(output_dir, exist_ok=True)
+def generate_rst_from_argcheck(output_dir=None):
+ base = Path(__file__).resolve().parents[2] # repo root
+ out_dir = Path(output_dir) if output_dir else (base / "docs" / "input_params")
+ out_dir.mkdir(parents=True, exist_ok=True)
@@
- with open(os.path.join(output_dir, f"{name}.rst"), "w", encoding="utf-8") as f:
+ with open(out_dir / f"{name}.rst", "w", encoding="utf-8") as f:
f.write(rst)📝 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 generate_rst_from_argcheck(output_dir="docs/input_params"): | |
| os.makedirs(output_dir, exist_ok=True) | |
| # 这里你可以写死，也可以动态扫描 | |
| modules = { | |
| "common_options": argcheck.common_options, | |
| "run_options": argcheck.run_options, | |
| } | |
| import os | |
| from pathlib import Path | |
| def generate_rst_from_argcheck(output_dir=None): | |
| base = Path(__file__).resolve().parents[2] # repo root | |
| out_dir = Path(output_dir) if output_dir else (base / "docs" / "input_params") | |
| out_dir.mkdir(parents=True, exist_ok=True) | |
| # 这里你可以写死，也可以动态扫描 | |
| modules = { | |
| "common_options": argcheck.common_options, | |
| "run_options": argcheck.run_options, | |
| } |
🧰 Tools
🪛 Ruff (0.12.2)
43-43: Comment contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF003)
🤖 Prompt for AI Agents
In dpnegf/utils/gen_input_docs.py around lines 40 to 47, the default output_dir
"docs/input_params" is relative to CWD and will become docs/docs/input_params
when called from docs/conf.py; change the default to be resolved relative to the
repository root (e.g. compute repo_root = Path(__file__).resolve().parents[2]
and set default output_dir = str(repo_root / "docs" / "input_params")) or
require/validate an explicit absolute path passed in; update callers
(docs/conf.py) to pass the explicit path if needed.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/hands_on/tutorial1_c_chain.ipynb (3)
351-361: Replacerm -rwith cross‑platform, safe deletion.
os.system('rm -r ...')is non‑portable and unsafe if paths contain spaces.-import os -... -if os.path.exists(results_path): - os.system('rm -r %s' % results_path) +import shutil +... +shutil.rmtree(results_path, ignore_errors=True)
304-321: Remove committed notebook outputs and guard TBPLaS usageVerified: docs/hands_on/tutorial1_c_chain.ipynb contains committed outputs (verification: outputs_committed=True, assertion raised).
- Clear outputs before committing:
jupyter nbconvert --clear-output --inplace docs/hands_on/tutorial1_c_chain.ipynb(or usenbstripout/ pre-commit hook).- Add a runtime guard to skip TBPLaS-dependent steps when the package is absent:
import importlib.util if importlib.util.find_spec("tbplas") is None: print("TBPLaS not installed; skipping TBPLaS-dependent steps.") else: # proceed with TBPLaS-dependent codeRe-run the verification script to confirm outputs are removed.
598-606: Replace notebook/shellrmand POSIX-only cells with Python stdlib (shutil/pathlib) or subprocess-with-args.Found remaining uses to fix (replace os.system/
!cells with shutil.rmtree / Path.unlink or subprocess.run([...], check=True), avoid shell=True or inline!):
- docs/hands_on/tutorial1_c_chain.ipynb —
!tree -L 1 ./(line ~170),os.system('rm -r %s' % results_path)(line 352),os.system('rm -rf %s' % output)(line 400),os.system('rm -r %s' % output)(line 570),! [ -d "../negf_output_cli" ] && rm -r ../negf_output_cli(line 605).- dpnegf/tests/test_negf_run.py —
os.system("rm -r "+output+"/results")at lines 35, 103, 170.- examples/atomic_chain_api/run.ipynb —
os.system('rm -rf %s' % output)(line 52).- examples/CNT/run.ipynb —
os.system('rm -rf %s' % output)(line 52).- docs/hands_on/tutorial1_base_sk.ipynb —
!tree -L 1 ./(lines ~69, ~315).Use shutil.rmtree(Path(...)) or pathlib methods for removals; convert display
!treecells to Python listings or remove.
🧹 Nitpick comments (7)
docs/hands_on/tutorial1_c_chain.ipynb (7)
167-171: Avoid shell-only commands in notebooks.
!treeandos.chdir()reduce portability and break on Windows/CI. Prefer pathlib and Python listing.- workdir='../../examples/atomic_chain_api/input_files' - os.chdir(workdir) - !tree -L 1 ./ + from pathlib import Path + workdir = Path(__file__).resolve().parents[2] / "examples" / "atomic_chain_api" / "input_files" + for p in sorted(workdir.iterdir()): + print(p.name)Would you like me to replace other shell invocations similarly?
182-195: Ensure result/log directories exist; avoid implicit creation.
set_log_handles(..., Path(log_path))assumes parent dirs exist.results_path = '../band_plot' log_path = results_path+'/log' log_level = logging.INFO +Path(results_path).mkdir(parents=True, exist_ok=True) +Path(log_path).mkdir(parents=True, exist_ok=True) set_log_handles(log_level, Path(log_path) if log_path else None)Does
dpnegf.utils.loggers.set_log_handlesalready create directories? If yes, we can skip the mkdirs.
399-403: Use context manager for JSON; avoid shellrm -rf.Minor robustness/readability improvement.
-import os -... -output = "../negf_output" -if os.path.exists(output): - os.system('rm -rf %s' % output) -os.makedirs(output) -negf_json = json.load(open(negf_input_file)) +import shutil, os +output = "../negf_output" +shutil.rmtree(output, ignore_errors=True) +os.makedirs(output, exist_ok=True) +with open(negf_input_file, "r") as f: + negf_json = json.load(f)
569-582: Idempotent, portable cleanup before compute().Same
rm -rissue; also useexist_ok=Truefor mkdir.-if os.path.exists(output): - os.system('rm -r %s' % output) -os.makedirs(output) +import shutil, os +shutil.rmtree(output, ignore_errors=True) +os.makedirs(output, exist_ok=True)
597-607: Gate CLI invocations; suggest Python fallback.
! dpnegf run ...will fail if CLI isn’t installed or on non‑POSIX shells. Provide a guarded subprocess call.-! pwd -! [ -d "../negf_output_cli" ] && rm -r ../negf_output_cli -! dpnegf run negf_chain_new.json -i nnsk_C_new.json -stu chain.vasp -o ../negf_output_cli +import shutil, subprocess, shlex +from shutil import which +print(os.getcwd()) +shutil.rmtree("../negf_output_cli", ignore_errors=True) +if which("dpnegf"): + subprocess.run(shlex.split( + "dpnegf run negf_chain_new.json -i nnsk_C_new.json -stu chain.vasp -o ../negf_output_cli" + ), check=True) +else: + print("dpnegf CLI not found; skipping CLI run.")
628-634: Torch load: consider weights_only=True (security), or document why not.If only tensors/containers are stored,
weights_only=Truemitigates pickle risks (PyTorch ≥2.4). If the file includes custom objects, keep as-is but note it.-negf_out = torch.load(results_path,weights_only=False) +# If compatible with your saved format: +# negf_out = torch.load(results_path, weights_only=True) negf_out = torch.load(results_path, weights_only=False)Would you like to switch and test in CI?
598-607: Purge committed outputs to keep repo lean.Several cells still include large images/stdout/stderr. Clear outputs and set execution_count to null before committing.
- Add a pre-commit hook or CI step:
+ pre-commit: + - repo: https://github.com/kynan/nbstripout + rev: 0.6.1 + hooks: + - id: nbstripoutOr use:
jupyter nbconvert --ClearOutputPreprocessor.enabled=True --inplace docs/hands_on/tutorial1_c_chain.ipynbAlso applies to: 298-333, 382-397, 500-565
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/hands_on/tutorial1_c_chain.ipynb(8 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
docs/hands_on/tutorial1_c_chain.ipynb
30-30: Found useless expression. Either assign it to a variable or remove it.
(B018)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: run-tests
🔇 Additional comments (3)
docs/hands_on/tutorial1_c_chain.ipynb (3)
8-8: Title tweak looks good.Clearer, more specific heading.
117-120: Nice DPNEGF–DeePTB relationship clarification.Accurate and helpful context for readers.
33-36: Docs nit: link to install instructions.“Detailed installation instructions can be found in README.” Consider linking to the rendered docs or README path.
[raise_nitpick_refactor]- Detailed installation instructions can be found in README. + Detailed installation instructions can be found in the README (e.g., docs/installation.md).
* update totorial to delete unnecessary log info. * add docs * update index.rst * add gen_input_docs * add tpye check * update gen_input_docs and conf.py * fix typos
1 participant
This pull request introduces improvements to the documentation and tutorial for DeePTB-NEGF, focusing on enhancing clarity, updating content, and adding tooling for auto-generating input parameter documentation. The most notable changes are the addition of an automated script for generating RST documentation from code, updates to the tutorial notebook for better context and readability, and minor restructuring of the documentation index.
Documentation Automation
dpnegf/utils/gen_input_docs.pyto automatically generate RST documentation for input parameters using introspection of argument definitions. This script formats arguments, types, defaults, and documentation into readable RST files for Sphinx.docs/conf.pyto optionally auto-generate input docs during documentation builds.Tutorial Notebook Improvements
docs/hands_on/tutorial1_c_chain.ipynbto clarify the role of DPNEGF and DeePTB, improving the explanation of how the model is used for quantum transport calculations. [1] [2]Documentation Structure
docs/index.rstto introduce an "Overview" section before "Key Features", improving the logical flow and readability of the documentation homepage.Summary by CodeRabbit
Documentation
Chores