Implement _collection_ref to link objects to their NumberedObjectCollection parent #867

digvijay-y · 2025-12-17T18:00:50Z

Pull Request Checklist for MontePy

Description

Added a _collection_ref as a weak_ref which points the object back to its collection.

Problems

deepcopy got messed up link "number already in use" another failed test due to surface conflicts.
_cell pointed to wrong cell objects.

Solution:

Rebuild cache after deepcopy instead of overwriting _collection_ref.
Added _set_cell method to halfspace

    100 objects -> 0.127 seconds
    500 objects -> 0.443 seconds
   1000 objects -> 0.937 seconds
   8000 objects -> 8.385 seconds

Details

import time
import montepy


class Collection(montepy.Cells):
  """Use the real Cells collection for testing"""

  pass


def dump_stats(n):
  collection = Collection()
  start = time.time()
  for i in range(n):
      cell = montepy.Cell(f"{i+1} 0 -1")
      collection.append_renumber(cell)
  end = time.time()
  print(f"{n:>7d} objects -> {end - start:.3f} seconds")


if __name__ == "__main__":
  print("Performance test: append_renumber with _collection weakrefs")
  print("=" * 60)
  for n_cells in [100, 500, 1000, 8000]:
      dump_stats(n_cells)
  print("=" * 60)

Fixes #849

General Checklist

I have performed a self-review of my own code.
The code follows the standards outlined in the development documentation.
I have formatted my code with black version 25.
I have added tests that prove my fix is effective or that my feature works (if applicable).

LLM Disclosure

Were any large language models (LLM or "AI") used in to generate any of this code?

Yes
- Model(s) used:
- Copilot
No

Documentation Checklist

I have documented all added classes and methods.
For infrastructure updates, I have updated the developer's guide.
For significant new features, I have added a section to the getting started guide.

First-Time Contributor Checklist

If this is your first contribution, add yourself to pyproject.toml if you wish to do so.

Additional Notes for Reviewers

Ensure that:

This PR fully addresses and resolves the referenced issue(s).
The submitted code is consistent with the merge checklist outlined here.
The PR covers all relevant aspects according to the development guidelines.
100% coverage of the patch is achieved, or justification for a variance is given.

📚 Documentation preview 📚: https://montepy--867.org.readthedocs.build/en/867/

MicahGale · 2025-12-18T15:55:20Z

Thanks for doing this. I'll probably look at this in the next few days.

MicahGale

I didn't fully evaluate how this works, but I think there are some design changes needed first.

montepy/surfaces/half_space.py

montepy/cell.py

montepy/mcnp_object.py

montepy/numbered_mcnp_object.py

montepy/numbered_object_collection.py

digvijay-y · 2026-01-05T13:40:52Z

// Ipynd files got updated with black

Moved _collection_ref to Numbered_MCNP_Object
Removed _set_cell methods
Fixed _number_validator to raise error
Created _link_to_collection method
Removed cache rebuild
Fixed try block error message

digvijay-y · 2026-01-05T13:56:12Z

Append to Collection:

Objects	Time (s)
100	0.0003
200	0.0006
500	0.0014
1000	0.0030
4000	0.0121
8000	0.0448

Append with Renumber:

Objects	Time (s)
100	0.0005
200	0.0010
500	0.0027
1000	0.0055
4000	0.0224
8000	0.0434

Number Changes (500 ops):

Objects	Time (s)
100	0.0048
200	0.0041
500	0.0043
1000	0.0043

Deepcopy Problem:

Objects	Time (s)
100	0.3575
200	0.4597
500	1.4791
1000	2.8051
4000	11.0377
8000	21.7734

Details

import copy
import time
import montepy


def benchmark_append_to_collection(n_objects: int) -> float:
    """Benchmark appending N objects to a fresh collection."""
    collection = montepy.Cells()
    cells = [montepy.Cell(f"{i+1} 0 -1") for i in range(n_objects)]

    start = time.perf_counter()
    for cell in cells:
        collection.append(cell)
    return time.perf_counter() - start


def benchmark_append_renumber(n_objects: int) -> float:
    """Benchmark appending N objects with renumbering."""
    collection = montepy.Cells()
    cells = [montepy.Cell(f"{i+1} 0 -1") for i in range(n_objects)]

    start = time.perf_counter()
    for cell in cells:
        collection.append_renumber(cell)
    return time.perf_counter() - start


def benchmark_number_changes(n_objects: int) -> float:
    """Benchmark changing object numbers."""
    problem = montepy.read_input("tests/inputs/test.imcnp")
    base_num = 10000
    for i in range(n_objects):
        cell = montepy.Cell(f"{base_num + i} 0 -1")
        problem.cells.append(cell)

    cells = list(problem.cells)
    start = time.perf_counter()
    for i in range(500):
        cell = cells[i % len(cells)]
        old_num = cell.number
        cell.number = old_num + 100000
        cell.number = old_num
    return time.perf_counter() - start


def benchmark_deepcopy(n_objects: int) -> float:
    """Benchmark deepcopy of a problem."""
    problem = montepy.read_input("tests/inputs/test.imcnp")
    for i in range(n_objects):
        cell = montepy.Cell(f"{1000 + i} 0 -1")
        problem.cells.append(cell)

    start = time.perf_counter()
    _ = copy.deepcopy(problem)
    return time.perf_counter() - start


def print_table(name: str, sizes: list[int], benchmark_func):
    """Run benchmark and print simple table."""
    print(f"\n{name}:")
    print(f"{'Objects':<10} | {'Time (s)':<12}")
    print(f"{'-'*10} | {'-'*12}")
    
    for size in sizes:
        elapsed = benchmark_func(size)
        print(f"{size:<10} | {elapsed:<12.4f}")


def main():
    print("=" * 40)
    print("MontePy _collection_ref Benchmark")
    print("=" * 40)

    sizes = [100, 200, 500, 1000, 4000, 8000]
    small_sizes = [100, 200, 500, 1000]

    print_table("Append to Collection", sizes, benchmark_append_to_collection)
    print_table("Append with Renumber", sizes, benchmark_append_renumber)
    print_table("Number Changes (500 ops)", small_sizes, benchmark_number_changes)
    print_table("Deepcopy Problem", sizes, benchmark_deepcopy)
    
    # Sanity check
    print(f"\n{'='*40}")
    print("Sanity Check:")
    problem = montepy.read_input("tests/inputs/test.imcnp")
    cell = list(problem.cells)[0]
    if hasattr(cell, "_collection_ref") and cell._collection is not None:
        print(f"  ✓ _collection_ref is working")
    else:
        print(f"  ✗ _collection_ref is NOT set")
    print("=" * 40)


if __name__ == "__main__":
    main()

MicahGale

This is a big improvement, and much more like what I was thinking. I have some more feedback, but it is not major.

Looking at the data you collected this does look like it is indeed linear (O(N)), so that's great that it is no longer O(N^2). I have been thinking more about how to present performance data recently. I think in general, rates should generally be used (e.g., objects/second) over just raw time. My last paper on this though did not use rates, so I'm working on this as well.

MicahGale · 2026-01-05T22:30:23Z

doc/source/changelog.rst


 **Features Added**

+* Implement _collection_ref to link objects to their NumberedObjectCollection parent (:issue:`867`).


I just made a commit to your branch to re-organize the changelog. This won't be in 1.2.0, obviously because that ship has sailed. I haven't decided yet if this will be in 1.2.1 or 1.3.0.

MicahGale · 2026-01-05T22:37:58Z

montepy/cell.py

+        result._collection_ref = None
+        result._problem_ref = None


This is still not the desired behavior in all cases. Generally they should be added to the same problem. I know that this can lead to a number collision in an edge case with hypothesis, that I haven't fixed yet, and I just ignore it with rm -r .hypothesis. I think that's a separate issue from this, and I need to open a bug report. Try it without these lines, and with purging the .hypothesis folder.

MicahGale · 2026-01-05T22:39:06Z

montepy/numbered_mcnp_object.py

-        collection = getattr(self._problem, collection_type.__name__.lower())
+
+    # Only validate against collection if linked to a problem
+    if self._problem is not None:


You can skip a level of ifs and just check if self._collection is not None.

MicahGale · 2026-01-05T22:40:17Z

montepy/numbered_mcnp_object.py

+        else:
+            # Find collection via _problem
+            obj_map = montepy.MCNP_Problem._NUMBERED_OBJ_MAP
+            collection_type = obj_map.get(type(self))
+
+            if collection_type is None:
+                # Finding via inheritance
+                for obj_class in obj_map:
+                    if isinstance(self, obj_class):
+                        collection_type = obj_map[obj_class]
+                        break
+
+            if collection_type is not None:
+                collection = getattr(self._problem, collection_type.__name__.lower())
+            else:
+                raise TypeError(
+                    f"Could not find collection type for {type(self).__name__} in problem."
+                )
+


I think you can delete this whole branch. I don't see a case where self._problem is linked, but self._collection is not.

MicahGale · 2026-01-05T22:41:10Z

montepy/numbered_mcnp_object.py

+        if "_collection_ref" in state:
+            del state["_collection_ref"]


Good code. Could also use state.pop("_collection_ref", None)

MicahGale · 2026-01-05T22:46:45Z

montepy/numbered_mcnp_object.py


+    @property
+    def _collection(self):
+        """Returns the parent collection this object belongs to, if any."""


You need to also reimplement link_to_problem here. It should at least:

call super().link_to_problem

grab the relevant collection from problem. See the logic above in _validate_number.

create a weakref.ref to that collection (in _collection_ref).

This will be the secondary way to update this ref, besides appending.

Though this may lead to weird situations where objects are linked to problems without being in the collections. I'm going to belay this until I read further.

MicahGale · 2026-01-05T22:50:32Z

montepy/numbered_object_collection.py

            return self._problem_ref()
        return None

+    def _link_to_collection(self, obj):


Sorry this is the flip of what I meant.

I think these should be functions of NumberedMCNP_Object, and then called from _internal_append.

The reason being is objects should respect each other's private attributes. I am ok with calling each other's private functions as that in my mind is more marking it as internal use only.

MicahGale · 2026-01-05T22:50:49Z

montepy/numbered_object_collection.py

                )
        self.__num_cache[obj.number] = obj
        self._objects.append(obj)
+        self._link_to_collection(obj)


This should be obj._link_to_collection(self)

MicahGale · 2026-01-05T22:56:50Z

montepy/numbered_object_collection.py

        """
        self.__num_cache.pop(obj.number, None)
        self._objects.remove(obj)
+        self._unlink_from_collection(obj)


also obj._unlink_from_collection(self).

digvijay-y added 2 commits December 17, 2025 17:22

weakref

83b61f7

Merge branch 'idaholab:develop' into weakref

ae3e6c1

digvijay-y changed the title ~~Weakref : Collection_ref~~ Implement _collection_ref to link objects to their NumberedObjectCollection parent Dec 17, 2025

Merge branch 'develop' into weakref

622632d

MicahGale requested changes Dec 25, 2025

View reviewed changes

changes to collection_ref

3ca4d87

digvijay-y requested a review from MicahGale January 5, 2026 13:59

Update changelog.rst with release staging.

de5c480

MicahGale requested changes Jan 5, 2026

View reviewed changes


		Features Added

		* Implement _collection_ref to link objects to their NumberedObjectCollection parent (:issue:`867`).

Implement _collection_ref to link objects to their NumberedObjectCollection parent #867

Are you sure you want to change the base?

Implement _collection_ref to link objects to their NumberedObjectCollection parent #867

Uh oh!

Conversation

digvijay-y commented Dec 17, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Checklist for MontePy

Description

Problems

Solution:

General Checklist

LLM Disclosure

Documentation Checklist

First-Time Contributor Checklist

Additional Notes for Reviewers

Uh oh!

MicahGale commented Dec 18, 2025

Uh oh!

MicahGale left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

digvijay-y commented Jan 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

digvijay-y commented Jan 5, 2026

Uh oh!

MicahGale left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

digvijay-y commented Dec 17, 2025 •

edited by github-actions bot

Loading

digvijay-y commented Jan 5, 2026 •

edited

Loading