fix(surface): preserve original case of surface type mnemonics#929
fix(surface): preserve original case of surface type mnemonics#929novavale wants to merge 4 commits intoidaholab:developfrom
Conversation
Fixes idaholab#522 MCNP surface mnemonics are case-insensitive, so a user may write 'sO' or 'So' instead of 'SO'. Previously MontePy uppercased the token in convert_to_enum(switch_to_upper=True), which caused the original case to be lost when the problem was written back out. Root cause: convert_to_enum did not set _og_value after resolving the enum, so ValueNode._value_changed always returned True (since _og_value was None), causing format() to re-format via the enum's .value (always uppercase) instead of returning the preserved original token. Fix: set self._og_value = self._value at the end of convert_to_enum. This mirrors the pattern in convert_to_int (line 1011). When the user has not changed the surface type, _value_changed is False and format() returns the original token verbatim, preserving case. Tests: 7 parametrized cases in test_surface_preserves_mnemonic_case (so, sO, So, SO, pz, PZ, Pz). 540 tests pass; 1 pre-existing failure in test_cells_clone (unrelated hypothesis flakiness, documented).
MicahGale
left a comment
MicahGale
left a comment
There was a problem hiding this comment.
A few questions. Also the failing tests need to be resolved first.
montepy/input_parser/syntax_node.py
Outdated
| if not (allow_none and self._value is None): | ||
| self._value = enum_class(value) | ||
| self._formatter = self._FORMATTERS[format_type].copy() | ||
| self._og_value = self._value |
There was a problem hiding this comment.
Shouldn't this have been set when the ValueNode is first initialized?
tests/test_surfaces.py
Outdated
| def test_surface_preserves_mnemonic_case(in_str, expected): | ||
| """Surface mnemonics should be written with the original case (issue #522).""" | ||
| surf = Surface(in_str) | ||
| result = surf.format_for_mcnp_input((6, 2, 0))[0] |
Per MicahGale's review: 1. '_og_value should be set when ValueNode is first initialized' — Reverted the convert_to_enum change. __init__ already sets _og_value = self.value (the raw string token). Instead, extended _value_changed to handle enum types: when _type is an Enum subclass and the canonical value matches _og_value case-insensitively (e.g. SurfaceType.SO vs 'sO'), the field is treated as unchanged and format() returns the original token verbatim. 2. 'Use surf.mcnp_str()' — Updated test to call surf.mcnp_str() instead of surf.format_for_mcnp_input((6, 2, 0))[0]. 3. Added changelog entry under #Next Version# / Bugs Fixed. Note: doc-build 'broken hyperlinks' failure is pre-existing on develop (confirmed by checking develop branch CI runs) and unrelated to this PR.
…fix/preserve-surface-case
|
Thanks for the review — addressed in 4a10187. "Shouldn't this have been set when the You're right. Instead, I extended "Use Updated. Failing tests
|
| if isinstance(self._og_value, str) | ||
| else str(self._og_value) | ||
| ) |
There was a problem hiding this comment.
What cases are you guarding against for _og_value not being a string? None was previously checked.
|
In the failed tests it appears you can't always assume you are working with a string based |
2 participants
Description
Fixes #522
MCNP surface type mnemonics are case-insensitive, so users may write
sO,So, orsoinstead ofSO. Previously MontePy always wrote them back in uppercase, silently changing the user's input.Root cause:
ValueNode.convert_to_enum(switch_to_upper=True)did not set_og_valueafter resolving the enum. As a result,_value_changedalways returnedTrue(because_og_valuewasNone), causingformat()to fall through to the enum-formatting path — which emitsvalue.value(always uppercase, e.g."SO") instead of returning the original token verbatim.Fix: one line in
convert_to_enum— setself._og_value = self._valueafter the enum is resolved. This mirrors the identical pattern inconvert_to_int(line 1011). When the user hasn't changed the surface type,_value_changedisFalseandformat()returns the original token, preserving case.Before:
After:
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
Additional Notes for Reviewers
syntax_node.py:convert_to_enum— minimal surface areatest_cells_clone(known hypothesis flakiness, unrelated to this change)convert_to_enum, which in MontePy is called fromsurface.py(surface type) andlattice_input.py(lattice type, integer-based). Both benefit from the same case/value preservation logic.📚 Documentation preview 📚: https://montepy--929.org.readthedocs.build/en/929/