Implement more detailed `lt` for components. #1882

mgjarrett · 2024-09-16T01:57:34Z

What is the change?

Fall back to getCircleInnerDiameter for comparison when sorting components when getBoundingCircleOuterDiameter is equivalent.

Why is the change being made?

Components on a block are sorted by their getBoundingCircleOuterDiameter. Sometimes, two components have the same outer diameter, in which case the sorting done by __lt__ will depend on the order of comparison. i.e.,

a < b evaluates to False
b < a also evaluates to False
b == a evaluates to True, even though the two components are not necessarily identical, they only have one identical dimension.

sorted([a, b]) != sorted([b, a]) even though b and a are not identical.

Close #1875

Checklist

This PR has only one purpose or idea.
Tests have been added/updated to verify any new/changed code.

The code style follows good practices.
The commit message(s) follow good practices.

The release notes have been updated if necessary.
The documentation is still up-to-date in the doc folder.
The dependencies are still up-to-date in pyproject.toml.

drewj-tp

Of the 21 files changed here, maybe three of them are actually related to the heart of this PR and touch the __lt__ changes.

They're not bad changes per se, but they are nearly entirely import related changes: sorting of imports, breaking multiple imports, importing just the module not the things needed in the module. They just added extraneous diffs to this PR.

What can we collectively do to avoid this? There's probably some linter / formatting config we can add, maybe some "blessed" vs code related settings we can advertise. But, these changes make it hard to agree that "this PR has one idea or purpose" with the extra, unrelated changes

armi/reactor/blocks.py

armi/reactor/__init__.py

armi/reactor/components/__init__.py

drewj-tp · 2024-09-16T15:10:40Z

armi/reactor/components/__init__.py

+    def getCircleInnerDiameter(self, Tc=None, cold=False):
+        """
+        Approximate the component as circular; i.e., inner diameter is zero.
+
+        Parameters
+        ----------
+        Tc : float
+            Ignored for this component
+        cold : bool, optional
+            If True, compute the area with as-input dimensions, instead of thermally-expanded.
+
+        Notes
+        -----
+        Tc is not used in this method for this particular component.
+        """
+        return 0.0


This seems like an okay thing for a shaped component to have to define on their own. Maybe a DerivedShape is the only exception.

Saying this because we try/catch a NotImplementedError below so if Component.getCircleInnerDiameter is required for components, we'd be okay in that context.

But that could be a larger change that is likely better served outside this PR.

Non-blocking comment. If there's traction I can make a ticket

The assumption for the unshaped component is to return the smallest possible bounding circle outer diameter, i.e., assume the component has a disk shape. It seems consistent with that assumption to assume that the inner diameter is 0.0.

The exception is caught in __lt__, but only for the purpose of producing a clearer error message. The error is still raised and the code crashes. So we still need to be able to sort UnshapedComponents.

armi/reactor/components/volumetricShapes.py

armi/reactor/tests/test_components.py

mgjarrett · 2024-09-16T15:34:39Z

Of the 21 files changed here, maybe three of them are actually related to the heart of this PR and touch the __lt__ changes.

They're not bad changes per se, but they are nearly entirely import related changes: sorting of imports, breaking multiple imports, importing just the module not the things needed in the module. They just added extraneous diffs to this PR.

What can we collectively do to avoid this? There's probably some linter / formatting config we can add, maybe some "blessed" vs code related settings we can advertise. But, these changes make it hard to agree that "this PR has one idea or purpose" with the extra, unrelated changes

Sorry, I'm in the middle of working on this. The root of the problem is that I ran into a circular import, and all of the import changes are related to fixing the circular import. It started small and then I went down a rabbit hole -- I'll probably break those changes out into a separate PR now.

This reverts commit 9cb98e5.

This reverts commit cca4a14.

This reverts commit 0fd1632.

Fix getBoundingCircleOuterDiameter to return a diameter instead of a radius.

armi/reactor/components/__init__.py

john-science · 2024-09-16T20:22:38Z

armi/reactor/components/basicShapes.py

@@ -20,8 +20,7 @@
 """
 import math

-from armi.reactor.components import ShapedComponent
-from armi.reactor.components import componentParameters
+from armi.reactor.components.component import ShapedComponent, componentParameters


Mike, can we fix your IDE?

I don't particularly like these changes, and they make your PRs more muddled to read. Like... you didn't actually touch this file.

#sorry

Sure thing. I was able to turn it off, so I'll go through and revert these import changes.

doc/release/0.4.rst

Co-authored-by: John Stilley <[email protected]>

…nto component_sort

john-science · 2024-12-17T22:58:25Z

doc/release/0.5.rst

@@ -46,6 +46,7 @@ New Features
 #. New plugin hook ``beforeReactorConstruction`` to process settings before reactor init. (`PR#1945 <https://github.com/terrapower/armi/pull/1945>`_)
 #. Improving performance in the lattice physics interface by not updating cross sections at ``everyNode`` during coupled calculations. (`PR#1963 <https://github.com/terrapower/armi/pull/1963>`_)
 #. Allow merging a component with zero area into another component. (`PR#1858 <https://github.com/terrapower/armi/pull/1858>`_)
+#. Use inner diameter for sorting components when outer diameter is identical. (`PR#1882 <https://github.com/terrapower/armi/pull/1882>`_)


ARMI 0.5.0 was released.

Can we move this release note up to the 0.5.1 release?

Thanks!

john-science · 2024-12-17T22:59:16Z

@mgjarrett I believe I see a merge conflict.

This PR removes the file: doc/release/0.4.rst. We don't want to lose that file.

Thanks!

mgjarrett added 13 commits September 11, 2024 16:27

Implement more detailed __lt__ for components.

707aa88

Add a unit test for component sorting.

35b63a8

Update release notes.

6bc7dc1

Address linting error

1265d7c

Catch NotImplementedError

dbed10f

Remove all of the circular imports in components.

0fd1632

Fix broken unit tests.

cca4a14

Fix unit test reference result.

06f1ec1

Register RadialSegment component type.

37e8f0b

Black formatting.

af44807

Merge branch 'main' into component_sort

c65a0c3

Refactor more unit tests.

9cb98e5

Ignore unused import

d50b6dc

drewj-tp reviewed Sep 16, 2024

View reviewed changes

mgjarrett marked this pull request as draft September 16, 2024 15:34

mgjarrett added 14 commits September 16, 2024 08:59

Revert "Refactor more unit tests."

d1db895

This reverts commit 9cb98e5.

Revert "Fix broken unit tests."

6c891fd

This reverts commit cca4a14.

Revert "Remove all of the circular imports in components."

914fe58

This reverts commit 0fd1632.

Clear merge conflict.

88feb80

Revert trivial change.

0b1b6ef

Add getCircleInnerDiameter for UnshapedComponent.

12980cd

Fix circular import.

a62fd41

Black formatting.

acbf2fc

Update docstring.

53780e4

Address review comments.

2a7e4a1

One more review comment.

64d1d09

Remove commented code.

37e4310

Remove kwargs

80d57ab

Black formatting

9457b49

mgjarrett added 2 commits September 16, 2024 12:39

Fix namespace errors.

6dca45a

Add getCircleInnerDiameter to RadialSegment.

4e2234b

Fix getBoundingCircleOuterDiameter to return a diameter instead of a radius.

john-science added the feature request Smaller user request label Sep 16, 2024

john-science reviewed Sep 16, 2024

View reviewed changes

armi/reactor/components/__init__.py Outdated Show resolved Hide resolved

john-science reviewed Sep 16, 2024

View reviewed changes

doc/release/0.4.rst Outdated Show resolved Hide resolved

mgjarrett and others added 10 commits September 16, 2024 14:15

Update armi/reactor/components/__init__.py

48603a4

Co-authored-by: John Stilley <[email protected]>

Update doc/release/0.4.rst

3ccdc5b

Co-authored-by: John Stilley <[email protected]>

Revert to main blocks.py

2075f3a

Revert import change.

5b601f9

Merge branch 'component_sort' of https://github.com/terrapower/armi i…

15155be

…nto component_sort

Revert more import changes.

7733ad8

Revert more imports.

9b61363

Fix an import.

b87c1d1

Merge branch 'main' into component_sort

557a333

merging in main

0fb4219

john-science reviewed Dec 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement more detailed `lt` for components. #1882

Implement more detailed `lt` for components. #1882

mgjarrett commented Sep 16, 2024 •

edited by john-science

Loading

drewj-tp left a comment

drewj-tp Sep 16, 2024

mgjarrett Sep 16, 2024

mgjarrett commented Sep 16, 2024

john-science Sep 16, 2024

mgjarrett Sep 16, 2024

john-science Dec 17, 2024

john-science commented Dec 17, 2024

Implement more detailed __lt__ for components. #1882

Are you sure you want to change the base?

Implement more detailed __lt__ for components. #1882

Conversation

mgjarrett commented Sep 16, 2024 • edited by john-science Loading

What is the change?

Why is the change being made?

Checklist

drewj-tp left a comment

Choose a reason for hiding this comment

drewj-tp Sep 16, 2024

Choose a reason for hiding this comment

mgjarrett Sep 16, 2024

Choose a reason for hiding this comment

mgjarrett commented Sep 16, 2024

john-science Sep 16, 2024

Choose a reason for hiding this comment

mgjarrett Sep 16, 2024

Choose a reason for hiding this comment

john-science Dec 17, 2024

Choose a reason for hiding this comment

john-science commented Dec 17, 2024

Implement more detailed `lt` for components. #1882

Implement more detailed `lt` for components. #1882

mgjarrett commented Sep 16, 2024 •

edited by john-science

Loading