Add Tally Support ... Finally - #1005
Draft
MicahGale wants to merge 60 commits into
Draft
Conversation
DataLexer's FILE_PATH pattern consumes `[` and `]`, preventing them from being matched as literals. TallyLexer narrows FILE_PATH to exclude those characters and adds them as literals so `[0 0 0]` tokenizes correctly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds grammar rules for: - Path separators: (1<2<5) - Lattice element indices: (1[0 0 0]<2) - Lattice ranges: (1<2[0:1 0:1 0:0]<5) - Comma-separated lattice sets: (1<2[0 0 0, 0 1 0]<5) - Nested sub-paths: (1<(2[0 0 0] 2[0 1 0])<5) - Universe references: (u=1<2<5), ((u=1)<2<5) tally_numbers uses fresh rules (no inherited number_sequence) so the LALR(1) conflict between lparen_phrase and the parenthetical number_sequence production is eliminated — those states are unreachable from the `tally` start symbol. All new rule names are distinct from inherited DataParser rules to avoid MetaBuilder merge side-effects. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Six bugs prevented TallyParser (and TallySegmentParser) from ever being used; all tally inputs silently fell back to DataParser: 1. _load_correct_parser stored a parser instance instead of the class, so _parse_input's self._parser() call double-instantiated it. 2. DataInput.__init__ did not propagate _prefix when full_parse() called it without a prefix argument, so _load_correct_parser was skipped. 3. parse_data did not pass prefix= to the final DataInput() call. 4. Input.tokenize() had no lexer_class parameter, so parser-specific lexers (e.g. TallyLexer) could not be selected. 5. _parse_input did not forward parser._lexer_class to tokenize(). 6. DataInput._KEYS_TO_PRESERVE did not include _prefix, so the prefix was not guaranteed to survive the JIT-to-full-parse transition. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
TestTallyPathSyntax directly invokes TallyParser + TallyLexer against all 11 complex tally forms from test_tally.imcnp. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add a full Tally class hierarchy (SurfaceCurrentTally, SurfaceFluxTally, CellFluxTally, DetectorTally, EnergyDepositionTally, FissionEnergyDepositionTally, EnergyDetectorPulseTally) with JIT parsing, subclass dispatch via Tally.from_input(), and correct handling of path/ lattice syntax in tally groups. Key changes: - tally.py: full rewrite with TallyGroup, FlatGroup, PathGroup, LatticeIndex helpers; lattice phrase and universe phrase parsing; link_to_problem uses problem._surfaces/_cells to avoid __relink_objs recursion - tally_parser.py: fix tally_group_body to preserve lattice_phrase and universe_phrase ListNodes intact (only flatten ShortcutNode) - tallies.py: inherit from NumberedDataObjectCollection (not NumberedObjectCollection) so insert_in_data kwarg is supported - data_parser.py: register Tally and dispatch via Tally.from_input() - mcnp_problem.py: add tallies property and load tallies during parse - cell.py: add tallies generator property (scanning pattern) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…yParser. Three genuinely dead pieces, each verified unreachable before removal rather than assumed: - TallyParser.paren_phrase: never referenced as a grammar symbol by any other rule (confirmed via grep) -- the real grammar uses lparen_phrase/rparen_phrase inherited from parser_base.py. - _parse_tally_numbers's top-level PaddingNode skip: empirically, the grammar never produces a bare PaddingNode item at that level (padding always attaches to the preceding ValueNode); even if it somehow did, PaddingNode fails the ValueNode/ListNode checks that follow, so the explicit `continue` was already behaviorally redundant. - Tally._dispatch_class's None return path, and the two `if subclass is None` fallbacks it fed in from_input, plus clone_as's equivalent target_cls-is-None check: _TALLY_TYPE_MAP's keys are provably exactly TallyType's full membership (verified: set(TallyType) == set(_TALLY_TYPE_MAP.keys())), so a successful TallyType lookup can never miss the map. Simplified _dispatch_class to always return a concrete subclass, which let from_input collapse both call sites down to the same `return subclass(input, jit_parse=jit_parse)`. Existing test suite passes unchanged against these removals, confirming no reachable behavior depended on the removed branches. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tly returning False. __contains__ checked the JIT marker directly and short-circuited to False for a still-JIT-parsed tally, rather than triggering a full parse like every other data-bearing property on Tally does. In practice this meant `item in tally` silently read as False on a freshly-read (still-JIT) tally until something else happened to touch it first -- a correctness trap discovered while adding coverage for this method. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…object model. Brings montepy/data_inputs/tally.py, tally_multiplier.py, tally_multiplier_type.py, tally_type.py, tallies.py, and input_parser/tally_parser.py to 100% patch coverage. Most of what was uncovered turned out to be real, correct, reachable behavior that simply had no test yet -- not dead code, despite some of it initially looking that way: - Forcing a full parse of every tally already present in tests/inputs/test_tally.imcnp closes most of the previously-untouched path/lattice/universe parsing code (the fixtures existed, nothing exercised their semantic interpretation via Tally.groups). - The from-scratch builder API (add_surface/add_group/add_path_group, PathGroup.inside) had almost no coverage; only CellTally.add_cell did. - __repr__/__eq__-against-wrong-type smoke tests across every small value object in both files -- pytest only reprs on assertion failure, so passing tests never previously touched these lines. - A universe designator nested in its own parens inside a path-free tally group (f4:n (5 (u=2) 7)) exercises _extract_universe_spec_from_nodes's recursive branch, and resolves correctly. - A bare tally prefix with no number at all (Input(["f"])) reaches from_input's except-Exception fallback, which correctly surfaces a ParsingError via full construction. - clone_as(str) and clone_as(<a Tally subclass outside SurfaceTally/CellTally/DetectorTally>) both exercise real validation paths that @args_checked's type[Tally] annotation does not actually enforce at the type level. - Tally/TallyMultiplier's __str__/__repr__ except branches, exercised via __new__(), document their real purpose: reporting gracefully on an object that bypassed __init__ entirely. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Pure whitespace/line-wrap changes left uncommitted from earlier work; verified AST-identical to the prior committed version before applying. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Common reaction numbers (CAPTURE, N_2N, TOTAL, etc.) are now class attributes directly on Reaction, attached right after the class definition since Reaction can't reference itself inside its own class body. Removes the separate ReactionNumber class entirely. This shortens both the import (one name instead of two) and the DSL itself: `26 & Reaction.N_2N * Reaction.N_P` instead of `26 & ReactionNumber.N_2N * ReactionNumber.N_P`. Also fixes a batch of docstring cross-references in the same classes/methods that never resolved to anything: bare `:class:`/`:func:` references to `ReactionExpression`, `MultiplierSet`, `MultiplierBin`, and dunder methods (which aren't documented here and can't be linked to), replaced with fully-qualified paths or plain code formatting as appropriate. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…d user guide. Exports the full public surface of the Tally/TallyMultiplier feature at the top-level montepy namespace, matching how Cell/Material/Surface are already handled: all Tally subclasses (Tally, SurfaceTally, CellTally, DetectorTally, and the seven concrete F1Tally...F8Tally types), Tallies, TallyType, Score, TallyMultiplier, Reaction, MultiplierSet, AttenuatorLayer, AttenuatorSet, SpecialMultiplierSet, and SpecialMultiplier. Left nested (not top-level) whatever is never directly constructed by a user, only ever handed back from a factory or a read-only property: TallyGroup, Filter, FlatGroup, PathGroup, LatticeIndex, ParticleFilter, SpatialFilter (from .groups/.filters), ReactionExpression (users compose Reaction instances with operators instead of calling it directly), ReactionOperator, MultiplierScore, and MultiplierBin (both parsed output from an existing FM card, not built by hand). Adds a "Tallies" section to doc/source/api/modules.rst, placed right after "Materials", so every one of the above (top-level or not) actually gets a generated API reference page instead of silently having no docs target. Fixed a batch of docstring cross-references across tally.py/tally_type.py that never resolved (bare `:class:`/`:attr:` references to TallyGroup, ParticleFilter, SpatialFilter, PathGroup, and stale montepy.data_inputs.tally.Tally-style paths that need to be the new short montepy.Tally form to resolve now that Tally is registered under its top-level name in the API docs). Adds doc/source/guide/tallies.rst, a new user guide page covering the Tally object hierarchy, groups, building tallies from scratch, cloning, scores/filters, tally multipliers, the Reaction expression DSL, and universe/lattice paths. Wired into the guide toctree in starting.rst. Every code example was verified against live execution (exact output, not guessed), and cross-checked against a full Sphinx build to confirm every cross-reference actually resolves. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Tags every public class introduced by the Tally/TallyMultiplier object model with `.. versionadded:: 1.6.0b2`, per the mandatory docstring convention in doc/source/devguide/docstrings.rst. Confirmed separately that Tallies.append/finalize_init's empty docstrings correctly fall through to their parent class's docstrings via inspect.getdoc(), so no change was needed there. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…P_Problem.tallies. The readiness review found two new public properties in pre-existing files that never got tagged, unlike their sibling properties in the same files, and a missing changelog entry for the Tally/TallyMultiplier feature (fixes #11). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
MicahGale
changed the title
MicahGale
changed the title
Fixes #11's CI doc-build job, which runs Sphinx in nitpicky+strict mode: AttenuatorSet and MultiplierScore annotated their constructors with bare numbers.Real/Integral instead of the codebase's montepy.types (ty.Real/ty.Integral) aliases, so autodoc_type_aliases never remapped them to the resolvable numbers.Real/Integral targets. Switched every bare Real/Integral type hint in tally.py and tally_multiplier.py to the ty.* aliases, matching the rest of the codebase, and dropped the now-unused `from numbers import ...` imports. Also moves montepy.Tallies into the Collections section (alongside Cells/Materials/Surfaces) and splits the crowded flat Tallies API list into Tally Objects / Scoring Groups and Filters / Tally Multipliers subsections. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Micah badgered Claude for its poor writing: retired "F card"/"FM card"/ "card" language repo-wide in favor of "input" (per doc/source/developing.rst's own "punchcards are dead" note), replaced a DSL-jargon section heading with plain language, rewrote the tallies guide's intro and Tally Object Hierarchy section (table instead of a prose wall of subclasses), and fixed Tally/ TallyMultiplier.__str__ to include the resolved subclass name instead of a hardcoded "TALLY" literal, matching the rest of the codebase's convention. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… its API pages. Rewrites across doc/source/guide/tallies.rst: retitles "What a Tally Scores" to "Geometry Filters" with separate flat-vs-grouped explanations, tightens run-on sentences, reworks the tally-type list into a table, adds an OpenMC-terminology attribution note to "Scores and Filters", rebuilds "Building Reaction Expressions" around Reaction's named MT constants instead of raw numbers, and makes "Universe and Lattice Paths" build up incrementally instead of leading with a three-concept example. Also adds doc/source/_templates/mytallyclass.rst (autoclass without the shared :private-members: list) and points modules.rst's Tally/TallyMultiplier autosummary blocks at it, so tally API pages stop listing internal parser-plumbing methods. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The :manual63: role only resolves subsection-level anchors, but the References section pointed it at "5.9", a section-level heading -- a dead link. Point it at "5.9.1" (F: Standard Tallies) instead, which is both a real subsection and the part of the manual actually relevant to what this guide covers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds 507 named class-attribute constants to Reaction, transcribed directly from Appendix B of the ENDF-6 Formats Manual (500 constants, covering essentially every officially-assigned MT number: standard reaction channels, discrete-level/continuum blocks for (n,p)/(n,d)/ (n,t)/(n,He3)/(n,alpha)/(n,2n)/(n,n'), and atomic-subshell data) plus 7 NJOY2016-specific "MT" identifiers from its GROUPR/HEATR/DTFR modules that aren't official ENDF-6 numbers. Raw Reaction(n) construction is unaffected -- these are purely a convenience layer, matching the class's existing "not exhaustive or closed" design. Adds a structural test (distinct .number values across all constants) and two changelog entries under #Next Version#. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tries.
doc/source/guide/tallies.rst: Geometry Filters examples now resolve cells
and print real Cell objects instead of raw old_numbers ints; drops a
redundant Reaction.CAPTURE example already covered by the preceding
named-constants block; fixes a sentence fragment ("Scale the constant
afterwards with *:"); fixes two remaining "card" mentions missed in an
earlier pass.
doc/source/changelog.rst: drops the two Reaction-constant #Next Version#
entries -- already covered by the general Tally/TallyMultiplier object
model entry above them.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ParticleFilter previously copied particles into its own order-sensitive list, comparing/repr'ing by list identity even though its data actually came from an unordered set (ParticleNode.particles), making equality nondeterministic across process runs for any classifier with 2+ particles (:n,p vs :p,n could compare equal or not, purely by Python's hash-seed luck). Redesigned to mirror Mode: ParticleFilter now wraps the real ParticleNode directly (or synthesizes one from a plain iterable), treats particle membership as a set for __eq__ (frozenset comparison, order-independent), and only defers to the node's own order-aware format() when rendering __repr__. Updated Tally.filters to pass the actual ParticleNode instead of the already-unwrapped set, and updated the tallies guide's example output to match the new repr. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… an alias inconsistency. TallyLexer previously narrowed FILE_PATH's regex just enough to stop it swallowing lattice-bracket syntax ([0 0 0]), but the token itself is grammatically unreachable from TallyParser -- nothing in the tally grammar references the file_atom/file_name productions that are FILE_PATH's only consumers. Disable it outright with a regex that can never match ((?!)), rather than merely narrowing it, so no tally input can ever be silently misread as a file path. Also fixes mcnp_problem.py's _NUMBERED_OBJ_MAP to use the already-imported tally_mod.Tally alias instead of the fully-qualified montepy.data_inputs.tally.Tally, matching the convention every other entry in that dict already follows. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Tally._update_values was a no-op, so add_cell/add_group/add_path_group
only ever mutated an in-memory Python list -- mcnp_str() never
reflected them (a from-scratch tally even wrote out as an empty,
invalid card). FlatGroup/PathGroup now carry a real syntax-tree node,
built and patched via the same cache-then-patch pattern HalfSpace uses
(node property, _ensure_has_node, _update_node), and Tally._update_values
rebuilds the tally's number list from the current groups before
formatting. add_cell/add_group/add_path_group (both CellTally and
SurfaceTally) now also populate the new group's linked cells/surfaces
directly, so a later renumber is picked up live. FlatGroup gains public
cells_or_surfaces/lattice_indices accessors (closing the read-side of
the earlier-flagged FlatGroup accessor gap).
Fixes two pre-existing round-trip bugs surfaced by writing the first
real round-trip tests for this code (confirmed via git stash to predate
this work, not introduced by it):
* Interpolation shortcuts (e.g. "3i") were silently corrupted by
full_parse() -- tally_group_body/tally_numbers flattened ShortcutNode
into its expanded value nodes, discarding the object that knows how
to recompress back to "3i". Fixed by switching to `type() is ListNode`
instead of `isinstance`, matching the existing number_sequence
convention in parser_base.py for the identical problem.
* Path chains like "1<1" wrote back with inserted spaces ("1 < 1") --
ListNode.format()'s general "don't let adjacent values run together"
safety net doesn't know some tally syntax is legitimately tight.
Fixed by marking genuinely-adjacent nodes never_pad after parsing.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…the tree. TallyMultiplier._update_values was a no-op, so there was no way to add an FM bin and have it show up in mcnp_str() (bins was read-only besides). Reaction/ReactionExpression/AttenuatorSet/MultiplierSet/SpecialMultiplierSet/ MultiplierBin all gain a `.node` property. Unlike tally.py's groups, these are fully immutable value objects (no setters), so the design simplifies to "cache the whole original raw fragment once during parsing, or lazily build fresh once if constructed via the Python API" -- no per-field patching needed except where noted below. add_bin/remove_bin let you build a bin set from Reaction/MultiplierSet objects and have it actually appear in the FM card; `bins` stays a read-only view, mirroring Tally.groups/add_cell. TallyMultiplier._update_values() only rebuilds the FM body when the bin list actually changed since parsing (tracked via a _parsed_bins snapshot). This matters because FM parenthesization is sometimes genuinely optional/ stylistic (MCNP manual rule 3), unlike tally.py's grouping -- naively re-deciding wrap-or-not from term counts on every write wouldn't always match what the original author chose to write, so untouched FM cards are left alone entirely instead. Also fixes two more real bugs surfaced while writing round-trip tests for the first time: * The same never_pad/adjacency bug as the earlier tally.py fix: bare ":"/"#" reaction operators (e.g. "16:103") were getting a space inserted even on first construction. Needed a recursive version of the same fix here, since FM nesting (bins containing individually parenthesized reaction lists) goes deeper than tally.py's case. * MultiplierSet.material now accepts and resolves a live montepy.Material instead of eagerly flattening it to a bare number at construction time (mat & Reaction.N_2N keeps the object; .material resolves .number live, and the cached node's material field is repatched whenever accessed again, so a rename after the first write still shows up on the next one). __eq__/__repr__ updated to compare/show the resolved number. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…nd/link. Tallies.append's TallyMultiplier branch never called super().append() or touched data_inputs, so problem.tallies.append(fm) silently dropped the FM card from any written output. Tallies now tracks its own flat multipliers list (an FM's number isn't independently unique, so it can't join a real NumberedObjectCollection) and inserts into data_inputs itself; Tally.multiplier's setter routes through the same registration path via a validator.
…them. Deleting a Tally with a linked TallyMultiplier previously left the FM card behind in problem.data_inputs, producing invalid MCNP on write. Tallies._delete_hook now removes the linked FM and warns MalformedInputWarning, matching the existing duplicate-FM-cards warning precedent. Renumbering a tally now also pushes the new number onto its linked FM, since the FM's own number must always match its parent tally.
…ce, and TallyMultiplier.clone(). Tally.include_total and TallyMultiplier.include_total/cumulative were read-only despite being ordinary parsed flags; add setters (mutually exclusive on the FM side). CellTally/SurfaceTally gain remove_cell/ remove_surface/remove_group, mirroring add_cell/add_group/add_path_group, pruning the bookkeeping Cells/Surfaces collection only when no remaining group still references an item. TallyMultiplier.clone(tally=...) is a bespoke override since the generic Numbered_MCNP_Object.clone() can't find a collection to register a TallyMultiplier into.
…m-scratch classifier round-trip. Add the standard verify_export/verify_prob_export helper pair (matching test_surfaces.py/test_cell_problem.py) to test_tally.py and test_tally_multiplier.py, and wire them into existing round-trip tests plus new coverage for attenuator-only bins, single-vs-multi-bin parenthesization, and full-problem delete-cascade/renumber-sync export. The first verify_export run caught a real bug: a from-scratch Tally/ TallyMultiplier wrote an unparseable "F 4"/"FM 4" classifier (space between prefix and number) because the classifier number's -1 placeholder default permanently baked a reserved sign column into the node's formatter. Fixed by using a non-negative placeholder instead.
…ion docstring. Rewrite two stale doc claims that no longer match reality now that include_total and TallyMultiplier.bins are actually mutable, add examples for remove_cell/remove_group/remove_surface, cells_or_surfaces, lattice_indices, and TallyMultiplier.clone(), and reorganize the Reaction class docstring into a scannable category summary instead of one dense paragraph in front of an alphabetical 500+ attribute wall.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request Checklist for MontePy
Description
Adds a full object model for MCNP tally (
Fcard) and tally-multiplier (FMcard) inputs, so tallies can be read, inspected, built from scratch, cloned, and edited like any other MontePy object instead of being treated as opaque text.Highlights:
Tallybase class with a concrete subclass per tally type (SurfaceCurrentTally,SurfaceFluxTally,CellFluxTally,DetectorTally,EnergyDepositionTally,FissionEnergyDepositionTally,EnergyDetectorPulseTally), selected automatically from the F-card's type digit viaTally.from_input.<) chains, and lattice index ([i j k]) specifications, modeled asTallyGroup/FlatGroup/PathGroup/LatticeIndex.TallyMultiplier(FMcard) object model, including the reaction-number DSL (Reaction,ReactionExpression) with+/*/-operator overloading matching MCNP's FM reaction-list grammar, attenuator sets, and special multipliers.SurfaceTally.add_surface/add_group/add_path_group,CellTally.add_cell/add_group/add_path_group,PathGroup.insidefor chaining.Cell.talliesandMCNP_Problem.tallies(a newTalliescollection) for discovering/iterating tallies.Tally,TallyMultiplier,Reaction, allTallysubclasses,Tallies, andMultiplierSetare exported at the top level (montepy.Tally, etc.).Materialsection.Fixes #11
TODO
MultiplierSet__str__and__repr__MultiplierSet&construction to take a Material; not a number.TallyMultiplierbin is not settable withmat & Reaction.N_2N.MT=11102?ReactiondocumentationFCcommentsTbins are handled.Ttotal settable.TallyGroupnot havingcellsPathGroupNote on LLM Use
This was an experiment with "vibe coding". Besides what I wrote a few years ago I tried to do everything through Claude code. Though I feel like I was more involved than true "vibe coding".
Overall, while it did end decently, I still don't think it was worth it. I felt too detached from the design process, and not engaged enough that I felt confident with it. I will review all of the code first though.
I had to do a lot of micromanaging to get the user guide to actually be well written and well styled, and in my voice.
General Checklist
blackversion 25 or 26.LLM Disclosure
Are you?
Were any large language models (LLM or "AI") used in to generate any of this code?
Documentation Checklist
.. versionchanged::or.. versionadded::directives.Infrastructure Changes
Significant features or Behavior changes
First-Time Contributor Checklist
pyproject.tomlif you wish to do so.Additional Notes for Reviewers
Ensure that:
📚 Documentation preview 📚: https://montepy--1005.org.readthedocs.build/en/1005/