docs: 100 percent coverage of docstrings - #252
Conversation
|
Hi @leifdenby @joeloskarsson |
0d157e1 to
2fe4079
Compare
|
@Mohit-Lakra thanks for adding missing docstrings. Can I quickly ask how you differentiate between |
|
Hey @sadamov — fair point! To be honest, this PR was more of an initial small fix while I was getting familiar with the codebase. I picked the most visible modules first just to get a feel for the existing docstring style before touching everything. |
|
@sadamov |
Mohit-Lakra
changed the title
|
Hi @Shlok2305, I hope you're doing well. I see you've already made great progress on the documentation for Project #2. Following @sadamov's suggestion to avoid duplicate efforts (from my previous PRs #284 and #285), I was wondering if I could contribute to this PR and work alongside you to finalize the documentation? I have some detailed notes on the tensor shapes (e.g., (N, D)) and the hierarchical message-passing flow in HiLAM and BaseGraphModel that I’d love to integrate here if you're open to it. Please let me know if you're comfortable with me adding these as suggestions to your specific lines or sharing my docstrings here. Looking forward to your thoughts. Best regards, |
|
Hey @Junaid1206 — thanks for the kind words! The docstrings are now at 100% coverage across the codebase. If you spot any specific tensor shape annotations that could be improved, feel free to open a separate issue in my forked repo and I can address it in a follow-up PR. For now I'd like to keep this one focused and get it merged. 🙂 |
|
Hi @Mohit-Lakra, thanks for the update! I understand you want to keep this PR focused for a quick merge. @sadamov, since the current coverage in #252 is already at 100%, I will follow Mohit's suggestion and track specific tensor shape improvements separately. To add immediate unique value without conflicting with this PR, I’ll start documenting the metrics.py and graph_utils.py modules, which seem to be outside the current scope of #252. This way, we can achieve better overall project clarity. Thanks! |
This is great @Mohit-Lakra. Is it possible to get |
@leifdenby Great suggestion! interrogate supports a --fail-under 100 flag, so it can be used in pre-commit hook. Wiring it up soon — this will fail any commit that drops docstring coverage below 100%, keeping the coverage locked going forward. |
|
@leifdenby Updated the pre-commit now interrogate works and precommit fails if doc coverage is less than 100 percent Failed pre-commit
Successful pre-commit
|
|
For tracking: this is the canonical PR for docstring coverage going forward. Related work to fold in or coordinate with: #515 (@Omnagar - docstrings on |
|
@sadamov Yes, if you have capacity please do. There seems to always be something that gets in the way preventing me from getting to this 😓. |
… autoapi Takeover diff for mllam#252. Folds the verified findings from a multi-agent review pass (covering NumPy-style consistency, shape-name alignment vs the README glossary, Sphinx/autoapi readiness, docstring-vs-implementation accuracy, information preservation across the rewrite, and completeness beyond raw interrogate coverage) into one mechanical diff Mohit-Lakra can merge or cherry-pick. Headline changes: - Rename non-glossary tensor-shape names to canonical glossary forms across docstrings and inline comments (d_h -> hidden_dim, d_f -> num_state_vars, d_forcing -> num_forcing_vars, N_grid -> num_grid_nodes, N_edges -> num_edges, M_g2m / M_m2g / M_up / M_down / M_same / M_mesh -> num_edges, d_edge_f -> input_dim). - Tidy RST: wrap bare True/False in double backticks, convert # to RST section underlines, replace single backticks around shape strings with double backticks, remove em-dashes from README glossary. - Restore information the rewrite dropped: filename templates in npyfilesmeps/store, num_rec constraint in InteractionNet.aggregate, vrange "inferred from data" and projection mention in plot_spatial_error, full list of units in utils, hidden-dimension context in utils, equation context in base_graph_model. - Add missing Raises sections where the code raises (setup_training_logger, _get_heatmap_color_values, prepare_clamping_params, get_metric, WeatherDataset.create_dataarray_from_tensor, MDPDatastore __init__ / coords_projection, stack_grid_coords). - Cross-cutting: add CHANGELOG entry referencing mllam#252, add [tool.interrogate] block to pyproject.toml documenting the gate, switch the pre-commit hook to read that config (`-c pyproject.toml`) so local interrogate runs match CI. Format choices locked in for the project: - prose-Shape (`Shape ``(B, ...)``.`) over bullet-Shape; matches the dominant pattern already in the codebase. - numpydoc literal-set (`aggr : {"sum", "mean"}`) over prose enumeration. - Glossary stays at the current 12 entries; renames pick the closest existing canonical (num_edges rather than num_g2m_edges et al). Not in this PR (open follow-ups, discussed in the PR description): - utils.py docstring restructure / dropped-context restores (the load_graph Returns block, the Joel-flagged context drops). Apply agent died before completing this file due to session budget; the v2 review draft at /tmp/pr252_review_draft_v2.md has the exact text to restore. - plot_graph.py and train_model.py docstring polish (low-priority, no behaviour change needed). - whether to extend the interrogate gate to `tests/` (currently excluded via [tool.interrogate]). refs mllam#252 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Resolve 13 conflicts to combine pr252-review's docstring overhaul with main's recent changes (mllam#239 GPU normalization, mllam#507 PropagationNet and per-edge-type GNN selection, mllam#472 BufferList slice support, mllam#447/mllam#455/mllam#622/mllam#625/mllam#631/mllam#647 type-hint additions, plus the interaction_net to gnn_layers rename). Resolution rules: - Keep main's NEW code (type hints, new params, new methods). - Keep pr252-review's NumPy docstrings and dim-name glossary alignment. - Drop the standardize parameter and supporting code from WeatherDataset since mllam#239 moved standardization to GPU. Post-merge cleanups: - Add docstrings to PropagationNet.__init__, AdaptiveHelpFormatter (class + __init__), and the BufferList.__getitem__ @overload stubs so interrogate still hits 100% coverage. - Rename main-introduced N_grid / d_features in the WeatherDataset.__getitem__ Returns block to glossary names (num_grid_nodes, num_state_vars, num_forcing_vars). - Strip the unused typing.Union import from vis.py left over from the PEP 604 modernization in pr252-review. - Drop em-dash from tests/test_prediction_model_classes.py docstring. Pre-commit + interrogate (100%) all green. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Pull mllam/main commit 42f9b3a (mllam#580 consolidates training outputs into runs/<run-name>/). Two doc conflicts resolved: - CustomMLFlowLogger.__init__: merge the new save_dir parameter into the existing NumPy docstring so the param is documented under Parameters. - utils.setup_training_logger: merge the new run_dir parameter into the signature (our Parameters block already documented it). Also fix a stray bullet-continuation alignment in NpyFilesDatastoreMEPS.get_xy that was off by 6 spaces. Interrogate stays at 100% and pre-commit is clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Hi @Mohit-Lakra could you quickly go over the changes proposed in Mohit-Lakra#6 and if you agree merge that PR into your branch here? The plan would be to get #252 up-to-date and then give all reviewers until end of the week to ask for changes. On Friday we merge 🥳 sounds good? |
Takeover: align mllam#252 with README glossary, merge 25 main commits, address Joel review
There was a problem hiding this comment.
this was a Herculean effort, thanks so much @Mohit-Lakra and everyone who contributed!
…ring @observingClouds flagged that the second (shorter) ASCII folder tree is an incomplete repetition of the detailed one above. The detailed version is the source of truth; remove the duplicate. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
joeloskarsson
left a comment
joeloskarsson
left a comment
There was a problem hiding this comment.
All my comments from earlier have been fixed and I am happy. This is such a huge improvement to the codebase, big shoutouts to everyone involved in this! A really valuable contribution.
I love the shape glossary in the README. An idea I had was to explicitly reference this in the CONTRIBUTING.md file, added in #407 (can either be added in there or here, depending on which goes in first).
|
I HAVE MERGED |
15 participants
Summary
Improves docstring coverage and quality across core neural-lam modules as
groundwork for the API reference documentation system described in Issue #61.
Changes
neural_lam/__init__.py: Added module-level docstringneural_lam/datastore/__init__.py: Added module-level docstringneural_lam/models/__init__.py: Added module-level docstringneural_lam/config.py: ConvertedNeuralLAMConfigandInvalidConfigErrorto full NumPy-style docstrings
neural_lam/metrics.py: Convertedmask_and_reduce_metricandwmseto full NumPy-style docstrings with Parameters/Returns sections
neural_lam/interaction_net.py: Added complete NumPy-style docstring toInteractionNet.__init__with all parameters documentedMetrics
Motivation
This PR is the first step toward the structured documentation system
requested in Issue #61. Clean, consistent NumPy-style docstrings are a
prerequisite for
sphinx-autoapito generate a useful API reference —the Jupyter Book PR (coming next) depends on this baseline quality.