Introduce new Spectra and FispacOputput class#193
Conversation
WalkthroughReorganizes imports into a new Changes
Sequence Diagram(s)(omitted — changes are self-contained APIs and import migrations, not multi-component sequential flows) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/f4enix/output/decay_pathways.py (1)
100-100:⚠️ Potential issue | 🟡 MinorStale module path in docstrings.
Lines 100 and 157 reference
f4enix.material_library.AVAILABLE_MATERIALS, but the module has moved tof4enix.core.material_library.Proposed fix
- available materials are at f4enix.material_library.AVAILABLE_MATERIALS, + available materials are at f4enix.core.material_library.AVAILABLE_MATERIALS,(Apply in both
get_pathwaysandfilter_pathwaysdocstrings.)docs/source/examples/output/jupyters/pathwaylib.ipynb (2)
38-39:⚠️ Potential issue | 🟡 MinorStale module path in markdown cell.
Line 39 still references
f4enix.material_librarybut the module has moved tof4enix.core.material_library.
70-76:⚠️ Potential issue | 🟡 MinorCached notebook output is stale after the
CUCRZRrename.The output on Line 74 shows
MaterialComposition(Copper Cromium Zirconium (CuCrZr))butCUCRZR.nameis now"CuCrZr". Re-run the notebook to update the cached output.docs/source/examples/input/jupyters/d1suned.ipynb (1)
170-176:⚠️ Potential issue | 🟡 MinorStale notebook output references the old import path.
The output at line 174 still shows
<class 'f4enix.input.irradiation.Nuclide'>from a previous execution, but the import has been changed tof4enix.core.irradiation. Re-run this cell so the output reflects the updated module path.docs/source/examples/output/jupyters/fispact_legacy.ipynb (1)
354-367:⚠️ Potential issue | 🟡 MinorStale notebook output: double space in pathway string.
Line 357 shows
"Co59 -(n,g)-> Co60m -(IT)-> Co60\n"with two spaces beforeCo60. ThePathway.__str__method was updated in this PR (line 98 offispact_legacy_out.py) to remove the extra space. Re-run this cell to reflect the corrected output.
🤖 Fix all issues with AI agents
In `@src/f4enix/core/material_library.py`:
- Line 247: The material constant name was changed to "CuCrZr" but the CSV still
lists "Copper Cromium Zirconium (CuCrZr)", causing filter_pathways() (which
builds mat_names = [m.name for m in materials]) to not match and return empty
results; update src/f4enix/resources/global_filterable.csv to replace the old
display string (e.g., "Copper Cromium Zirconium (CuCrZr)") with the new material
name "CuCrZr" wherever it appears (line ~39 and any other rows), then
re-run/verify the filter_pathways() behavior and any tests that rely on material
matching to ensure consistency between the Material constant (CUCRZR / m.name)
and the CSV entries.
In `@src/f4enix/core/spectra.py`:
- Around line 164-177: Update the docstring for the to_arb_flux_file method:
replace the "_description_" placeholders with a clear description of the outdir
parameter (e.g. "outdir: PathLike — directory where the generated fispact
'arb_flux' file will be written; the method will create the file inside this
directory") and a clear return description (e.g. "Returns: Path — full path to
the written arb_flux file"). Also mention any side-effects or errors the caller
should expect (e.g. file creation and potential IO errors) and keep the
triple‑quoted docstring attached to to_arb_flux_file.
- Line 23: Fix the typo in the class docstring for the neutron spectra class:
change "expcted" to "expected" in the docstring string of the Spectra class (or
whatever class name defines "Class representing a neutron spectra. Values are
expcted as normalized to 1."). Ensure the revised docstring reads "Values are
expected as normalized to 1." while preserving the rest of the sentence.
In `@src/f4enix/input/MCNPinput.py`:
- Around line 49-52: Remove the duplicate import of Nuclide and METASTABLE_TAG
in the module import block: leave a single import statement importing Nuclide
and METASTABLE_TAG from f4enix.core.irradiation (remove the repeated "from
f4enix.core.irradiation import Nuclide, METASTABLE_TAG" line) so only one
occurrence of those symbols exists and the F811 redefinition error is resolved.
In `@src/f4enix/output/fispact_legacy_out.py`:
- Around line 416-424: The helper _get_zaid_from_str currently assigns isotope
from isotope_pat.search(name).group() as a str while FispactZaid.isotope is
annotated int; convert the captured isotope to int before constructing
FispactZaid (e.g., isotope = int(isotope_pat.search(name).group())) and keep the
metastable logic as-is; ensure you still call FispactZaid(element=element,
isotope=isotope, metastable=metastable) so the dataclass gets an int for
isotope.
- Line 234: The code currently calls line =
line.strip("\n").strip("\rn").strip(), where strip("\rn") incorrectly treats
"\rn" as backslash-r plus letter n and will remove literal 'n' characters;
change this to simply line = line.strip() or, if you need to only trim CR/LF,
use line = line.rstrip("\r\n") so you don't accidentally strip letters from
nuclide names; update the occurrence of strip("\rn") (the call on the variable
line) to one of these corrected forms.
- Around line 253-262: The "path continued" branch incorrectly parses the first
token as a ZAID (producing entries like FispactZaid(element='path',...));
instead skip the first token entirely and only parse subsequent ZAID tokens. In
the block handling the "path continued" line (the branch that builds tokens =
line.split("---") and currently calls
zaids.append(_get_zaid_from_str(tokens[0].split("%")[-1]))), remove that initial
append and iterate over tokens starting at the first real ZAID token (e.g., use
tokens[1], tokens[3], ... or a loop like for i in range(1, len(tokens), 2)) and
call _get_zaid_from_str on those tokens; update the existing loop indices
(currently using range(2, ...)) to begin at 1 and ensure you use len(tokens) as
the bound so only real ZAID tokens are appended to zaids.
In `@tests/fispact_legacy_out_test.py`:
- Around line 68-82: The test for PathwayCollection._parse_pathway only checks
parent, daughter, and last reaction but misses verifying intermediates from the
"path continued" segment; update the test_parse_pathway to assert the parsed
intermediates include the continuation nodes (e.g., ensure pathway.intermediates
contains "Pb212" in the expected position or that the full intermediate sequence
matches the expected list), referencing PathwayCollection._parse_pathway and the
pathway instance (pathway.intermediates, pathway.parent, pathway.daughter) so
the continuation parsing bug will be caught.
🧹 Nitpick comments (11)
tests/decay_pathways_test.py (1)
18-20: Commented-out test assertion should be tracked.This removes validation for the
dose=99pathway count. Consider opening an issue to track re-enabling this once the new data is in place.Would you like me to open an issue to track re-enabling this assertion?
src/f4enix/core/irradiation.py (1)
70-77:Pulse.__eq__includesunit, which may cause surprising inequality.Two pulses with the same physical duration (e.g.,
Pulse(60, 1.0, SECOND)andPulse(1, 1.0, MINUTE)) are stored identically (time=60s) but will compare as unequal becauseself.unit != value.unit. This could causescan_sequenceto miss valid repetitions if pulses in a scenario were constructed with mixed units.If the intent is physical equality, compare only
timeandintensity. If the intent is structural identity (including display preference), the current behavior is fine—but consider documenting it.src/f4enix/output/fispact_legacy_out.py (3)
82-91: Usingassertfor validation in__post_init__is fragile.
assertstatements are removed when Python runs with the-Oflag. For dataclass invariant checking, an explicitif/raiseis safer.♻️ Proposed refactor
def __post_init__(self): - # check that reaction list length is always one more than intermediates - # when it is not None if self.intermediates is not None: - try: - assert len(self.reactions) == len(self.intermediates) + 1 - except AssertionError as e: - raise AssertionError( - f"Number of reactions is too low for pathway {self}" - ) from e + if len(self.reactions) != len(self.intermediates) + 1: + raise ValueError( + f"Number of reactions ({len(self.reactions)}) does not match " + f"intermediates + 1 ({len(self.intermediates) + 1}) for pathway {self}" + )
80-80: Type hint onintermediatesshould includeNone.
Nonedefault doesn't match thelist[FispactZaid]annotation.♻️ Fix
- intermediates: list[FispactZaid] = None + intermediates: list[FispactZaid] | None = None
284-341:FispactOutputclass is a clean addition — nice integration of pypact with pathway collection.The
_get_sddrmethod andfilter_by_cum_doseprovide useful high-level operations. One minor suggestion: consider validating thatlen(cooling_times) <= len(self.inventory_data)in_get_sddrto fail early with a clear message if mismatched.tests/fispact_legacy_out_test.py (1)
107-111: Commented-out test should be removed or converted topytest.mark.skip/xfail.Leaving commented-out tests in the codebase obscures test coverage and intent.
src/f4enix/core/spectra.py (2)
148-162: f-strings without placeholders — remove extraneousfprefix.Lines 150 and 196 use f-strings with no interpolation. Flagged by Ruff (F541).
♻️ Fix
- text = f"""MONITOR 1 + text = """MONITOR 1 CLOBBER SPEK GETDECAY 1 FISPACT * CONDENSE END * END OF RUN """- f.write(f"1\n") # must have a wall load, likely not used + f.write("1\n") # must have a wall load, likely not usedAlso applies to: 196-197
70-81: Parameterformatshadows the Python builtin.Consider renaming to
fmtornumber_format.♻️ Suggested rename
- def to_fispact_fluxes(self, outdir: PathLike, format: str | None = None) -> None: + def to_fispact_fluxes(self, outdir: PathLike, fmt: str | None = None) -> None: ... - if format is None: - format = ".3e" + if fmt is None: + fmt = ".3e" with open(Path(outdir, self.name), "w") as f: for flux in np.flip(self.spectra_values): - f.write(f"{flux:{format}}\n") + f.write(f"{flux:{fmt}}\n")tests/spectra_test.py (3)
19-23: Unused variablespectra— consider adding an assertion or using_.The static analysis tool flags
spectraas assigned but never used. Either add a basic assertion (e.g., on the name or shape) to make this test more meaningful, or assign to_to clarify the intent is only to verify parsing doesn't raise.Suggested fix
def test_from_fispact(self, file): # check that spectra are read correctly from fispact files with as_file(RES.joinpath(file)) as fisp_file: - spectra = Spectra.from_fispact(GROUP_STRUCTURES["VITAMIN-J-175"], fisp_file) + spectra = Spectra.from_fispact(GROUP_STRUCTURES["VITAMIN-J-175"], fisp_file) + assert spectra.name == "FLUX1"
69-76:test_not_normalizedandtest_automatic_normalizationappear to test the same behavior.Both tests create a
Spectrawith unnormalized values and verify the sum is ~1.0. Consider consolidating them or differentiating the intent more clearly (e.g., one could test the warning emission).Also applies to: 102-106
41-45: Tests call methods but make no assertions on the output.
test_print_collapse_inpandtest_print_condensed_inponly verify no exceptions are raised. Consider adding at least a file-existence check on the generated output to add minimal validation.
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/f4enix/output/fispact_legacy_out.py (1)
104-147:⚠️ Potential issue | 🟡 MinorDocstring issues: typo and incorrect return type.
- Line 126:
patway.intermediates→pathway.intermediates- Line 147: Docstring claims the return type is
list[Pathway], butfrom_filereturnsPathwayCollection.Proposed fix
- ... patway.intermediates, pathway.perc) + ... pathway.intermediates, pathway.perc)Returns ------- - list[Pathway] - A list of Pathway objects representing the pathways found in the file. + PathwayCollection + A PathwayCollection representing the pathways found in the file.
🤖 Fix all issues with AI agents
In `@docs/source/examples/output/jupyters/rssa.ipynb`:
- Around line 43-44: The notebook has inconsistent execution_count values: the
first code cell's "execution_count" is null while other code cells (those
containing code outputs) still have integer execution counts; make them
consistent by either clearing execution counts for all code cells or restoring
integers for all—update every cell's "execution_count" field (search for
"cell_type": "code" and the "execution_count" key) so they all use the same
value (e.g., set every "execution_count": null) to ensure the notebook is
committed uniformly.
In `@src/f4enix/core/spectra.py`:
- Around line 53-59: The current normalization divides by np.sum(spectra_values)
without guarding against a zero sum; compute total = np.sum(spectra_values)
first, check if np.isclose(total, 0.0) and raise a clear ValueError (or log and
abort) when the sum is zero to avoid producing NaNs, otherwise if not
np.isclose(total, 1.0, atol=5e-3) log the warning and normalize via
spectra_values = spectra_values / total; reference the symbols spectra_values,
np.sum, np.isclose, logging.warning in your change.
- Around line 226-236: The loop can leave title_line unassigned and tests length
on the raw line; initialize title_line (e.g., title_line = "") before the loop,
use a stripped variable inside the loop (stripped = line.strip()) and check if
stripped and len(stripped) > 1 to assign title_line = stripped and break, and
after the loop handle the fallback when title_line is still empty (set a safe
default like "untitled" or an empty string) before running title_line =
title_line.replace(" ", "_"); this fixes the UnboundLocalError and ensures the
length check applies to the trimmed content.
In `@src/f4enix/output/fispact_legacy_out.py`:
- Around line 316-320: In _get_sddr, guard against an empty cooling_times list
so self.inventory_data[-len(cooling_times):] doesn't become
self.inventory_data[:] (because -0 == 0) and process all timesteps; add an early
check in _get_sddr (using the cooling_times parameter and the inventory_data
slice logic) to return an empty pd.DataFrame (with the same expected columns) or
skip processing when cooling_times is empty, or alternatively iterate by zipping
cooling_times with the sliced inventory_data to ensure no iterations occur when
cooling_times is empty.
🧹 Nitpick comments (9)
docs/source/examples/output/jupyters/pathwaylib.ipynb (1)
44-44: Inconsistentexecution_countvalues across cells.Some cells have been reset to
null(e.g., lines 44, 67, 297) while others retain concrete values (lines 93, 128, 427). Consider clearing all outputs/execution counts before committing for cleaner diffs going forward.Also applies to: 67-67, 93-93
src/f4enix/output/decay_pathways.py (1)
94-105: Docstring references tof4enix.decay_pathwaysmay also need updating.Lines 94, 104 (and similarly 151, 161) still reference
f4enix.decay_pathways.AVAILABLE_SPECTRAandf4enix.decay_pathways.AVAILABLE_COOLING_TIMES. Since this file lives atf4enix.output.decay_pathways, these paths appear inconsistent — especially now that the material reference on line 100 was updated to the fully qualifiedf4enix.core.material_librarypath.📝 Suggested docstring path fix
- allowed spectra are available at f4enix.decay_pathways.AVAILABLE_SPECTRA, + allowed spectra are available at f4enix.output.decay_pathways.AVAILABLE_SPECTRA,- allowed times are available at f4enix.decay_pathways.AVAILABLE_COOLING_TIMES, + allowed times are available at f4enix.output.decay_pathways.AVAILABLE_COOLING_TIMES,(Apply the same fix in both
get_pathwaysandfilter_pathwaysdocstrings.)src/f4enix/core/spectra.py (4)
150-158: Unnecessaryf-prefix on string with no placeholders.The
textf-string here contains no interpolated expressions, unlikeprint_collapse_inp. Remove thefprefix.- text = f"""MONITOR 1 + text = """MONITOR 1 CLOBBER SPEK GETDECAY 1 FISPACT * CONDENSE END * END OF RUN """
196-196: Unnecessaryf-prefix (same issue as line 150).- f.write(f"1\n") # must have a wall load, likely not used + f.write("1\n") # must have a wall load, likely not used
292-307: Primary spectrum gets dashed linestyle when plotted with comparisons.When
add_spectrais provided,self(the primary spectrum at index 0) receivesLINESTYLES[0]which is"--"(dashed). Typically the primary series uses a solid line"-"for visual prominence. Consider starting additional spectra fromLINESTYLESwhile keeping the primary solid.Proposed fix
for i, spec in enumerate(to_plot): if lethargy: values = spec.get_by_lethargy() else: values = spec.spectra_values if len(to_plot) > 1: - linestyle = LINESTYLES[i] + linestyle = "-" if i == 0 else LINESTYLES[i - 1] else: linestyle = "-"
288-290: Minor: prefer unpacking over list concatenation.Per Ruff RUF005, use
[self, *add_spectra]instead of[self] + add_spectra.- to_plot = [self] + add_spectra + to_plot = [self, *add_spectra]src/f4enix/output/fispact_legacy_out.py (3)
80-91:intermediatestype annotation is inconsistent withNonedefault, andassert-based validation is fragile.Two things here:
The annotation
list[FispactZaid]doesn't reflect thatNoneis a valid value. Uselist[FispactZaid] | None = None(orOptional[list[FispactZaid]]).The
try: assert ...; except AssertionErrorpattern is silently skipped when Python runs with-O(which stripsassertstatements). Prefer a directif/raisefor validation logic.Proposed fix
- intermediates: list[FispactZaid] = None + intermediates: list[FispactZaid] | None = None def __post_init__(self): - # check that reaction list length is always one more than intermediates - # when it is not None if self.intermediates is not None: - try: - assert len(self.reactions) == len(self.intermediates) + 1 - except AssertionError as e: - raise AssertionError( - f"Number of reactions is too low for pathway {self}" - ) from e + if len(self.reactions) != len(self.intermediates) + 1: + raise ValueError( + f"Expected {len(self.intermediates) + 1} reactions but got " + f"{len(self.reactions)} for pathway {self}" + )
170-173: Magic number10for the look-ahead window is fragile.If a pathway has many continuations (>4), 10 lines may not be enough. Conversely, if pathways are packed tightly, the window may bleed into the next pathway (though the
PATHWAY_ENDbreak mitigates this). Consider computing the actual extent of each pathway block (e.g., scanning until the nextpath_id_patmatch or blank separator) instead of a hardcoded constant.
416-424: Unguarded.group()calls on potentiallyNoneregex matches.If
namecontains no alphabetic or numeric characters,element_pat.search(name)orisotope_pat.search(name)returnsNone, and.group()raisesAttributeError. While this is an internal helper with controlled inputs, a guard (or explicit error) would make debugging easier if upstream parsing ever passes unexpected data.
2 participants
Description
New capabilities have been added in the FISPACT workflows area. A new
Spectraclass allows to efficiently deal with spectra (parsing and plotting), while theFispactOutputintroducedpypactdependency to implement high level operations on FISPACT outputs.Notably, the PR also introduce a re-organization of the modules with the introduction of a
coresubmodule in addition to the already existinginputandoutputones. This may bring minor incompatibilities with previous F4Enix versions as the import paths have been modified for some modules.Additional dependencies introduced.
Type of change
Please select what type of change this is.
Other changes
Testing
Suitable CI tests have been added
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests