terrapower / armi

An open-source nuclear reactor analysis automation framework that helps design teams increase efficiency and quality
https://terrapower.github.io/armi/
Apache License 2.0
214 stars 82 forks source link

Fix getNeighboringCellIndices #1614

Closed mgjarrett closed 5 months ago

mgjarrett commented 5 months ago

What is the change?

The staticmethod to get neighboring cell indices in a StructuredGrid is implemented incorrectly:

https://github.com/terrapower/armi/blob/14c951a05214154a1e6bfb43c728aa714084cea7/armi/reactor/grids/structuredgrid.py#L379-L382

It looks like a typo; there is a 1 where there should be an i.

Also, a unit test is added to cover this function.

Why is the change being made?

The previous implementation is incorrect.


Checklist

opotowsky commented 5 months ago

Whoah nice catch! Will this affect internal integration testing?

john-science commented 5 months ago

Whoah nice catch! Will this affect internal integration testing?

Michael did that testing. This is ready for prime time.

drewj-usnctech commented 5 months ago

FWIW this existed prior to the refactoring in #1373

https://github.com/terrapower/armi/blob/da28372a240e80418139d1069317f546af9bcd8d/armi/reactor/grids.py#L929-L932

My intention there was just to copy code off Grid and move it to StructuredGrid. But I'm glad it has been discovered and fixed!

mgjarrett commented 4 months ago

Ah, I didn't look that closely. Didn't see the side-by-side diff because it was moved to a new file.

It looks like this had been around for over 5 years (probably much longer). I guess this shows how little exercise the Cartesian code gets.

image