Adding OpenmcReader and Pebble Classes #1

ZoeRichter · 2024-02-21T18:30:12Z

This is a relatively small commit, which mainly adds the basic Pebble and OpenmcReader classes.

For now, I'm leaving the recirc parameter in the Pebble object, and setting a default value. This is likely to change later.

pep8speaks · 2024-02-21T18:30:18Z

Hello @ZoeRichter! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2024-05-31 19:17:59 UTC

nsryan2

LGTM, a few minor changes to some wording, but everything looks good!

src/didymus/pebble.py

nsryan2 · 2024-02-21T19:24:03Z

src/didymus/reader.py

Is there a more descriptive name for this file that would work better? reader_openmc? or just readers?

I agree with @nsryan2's comment. I might offer mc_reader.py standing for monte carlo reader which could be OpenMC or Serpent.

@ZoeRichter if you plan to have more than one reader, you could consider borrowing some of @yardasol's code in Saltproc for reading data from Serpent/OpenMC (and others?) Maybe I'm misremembering the purpose of depcode @yardasol.

Regardless, the pattern I'm suggesting is to have a generic reader object and have the openmc or serpent specific readers inherit from it and use the super().__init__() function (see osier.Technology).

I was on the fence about this specific implementation, actually. I initially started by creating it the way you suggested - by having a parent reader that's generic, and child classes for code-specific readers. But as I was making it, I realized the ones I knew enough about to be able to implement right now didn't actually have args in common, so I made the reader(s) distinct. I think going the other way (converting didymus to a given code format) will work best in that parent-child set up, however. Do you think it would still be best to organize them under an empty parent class?

src/didymus/reader.py

nsryan2 · 2024-02-21T19:24:39Z

src/didymus/reader.py

+        Parameters
+        ----------
+        coord_array : array
+            Array of center coordinates, in x, y, and z, from


Suggested change

Array of center coordinates, in x, y, and z, from

Array of center coordinates, in x, y, and z from

src/didymus/reader.py

nsryan2 · 2024-02-21T19:28:07Z

src/didymus/reader.py

+        ----------
+        peb_array : array of Pebble objects
+            Array containing a unique Pebble object for each
+            coordinate provided in coord_array.


Maybe a little unclear here, is it an object for each x, y, z set or each x coordinate, each y coordinate, and each z coordinate? The former, right?

Correct, it's a pebble object for each set of (x,y,z) coordinates in coord_array (each set corresponds to a specific pebble's center). Do you think "...for each centroid coordinate provided..." would be a better descriptor (bouncing off of Sam's suggested change in a later comment)?

samgdotson · 2024-02-21T20:24:30Z

I wanna take a look before this merges, if that's okay!

Co-authored-by: Nathan Ryan <[email protected]>

samgdotson

Hi @ZoeRichter thanks for this PR! It's a nice short PR. I left a lot of suggestions and I hope you're not feeling overwhelmed. Most of them are simply stylistic preferences. I want to highlight a few key items, though:

Choosing variable names can be more of an art than a science. Here, it seems like you default to shortening words that are already pretty short. E.g., pebble --> peb. I think this is a bit "lossy" because it's not immediately obvious what a peb is to the casual reader. Now, if you had a LOT of info, for example, pebble_radius_cartesian_coordinates then maybe it would be appropriate to shorten peb. I digress.
I think you're going to need an init.py file as well. Especially if you start adding sub modules.
There isn't much functionality in this PR -- GREAT! What a perfect time to add a testing suite! I recommend adding tests now so that the suite is built up incrementally and you don't get overwhelmed later when you have a lot MORE functionality to test.

I'm excited to see where this goes! Also, @yardasol I'm curious if you agree with some/any of my comments.

src/didymus/pebble.py

src/didymus/reader.py

samgdotson · 2024-02-21T21:06:56Z

src/didymus/reader.py

+        peb_array = []
+        for i, coord, in enumerate(self.coord_array):
+            peb_array.append(
+                peb.Pebble(
+                    coord,
+                    peb_rad,
+                    mat_ids[i],
+                    uniq_ids[i]))
+
+        return peb_array


Suggested change

peb_array = []

for i, coord, in enumerate(self.coord_array):

peb_array.append(

peb.Pebble(

coord,

peb_rad,

mat_ids[i],

uniq_ids[i]))

return peb_array

peb_array = [Pebble(coord, pebble_radius, mat_ids[i], pebble_ids[i]) for i, coord in enumerate(self.coord_array)]

return pebble_array

List comprehensions are faster. Is every pebble going to be a unique material? Should mat_ids be part of the initialization of the OpenMCReader? It seems cumbersome (and error prone) to have users generate id's for each pebble themselves.

Each pebble will not necessarily be a unique material, I think it's more likely that you will have large numbers of the same pebble material (such as having a unique material description for each pass the pebbles have made through the core). I don't think having a default for the uniq_ids/pebble_ids is a bad thing, though.

samgdotson · 2024-02-21T21:16:22Z

src/didymus/reader.py

I agree with @nsryan2's comment. I might offer mc_reader.py standing for monte carlo reader which could be OpenMC or Serpent.

@ZoeRichter if you plan to have more than one reader, you could consider borrowing some of @yardasol's code in Saltproc for reading data from Serpent/OpenMC (and others?) Maybe I'm misremembering the purpose of depcode @yardasol.

Regardless, the pattern I'm suggesting is to have a generic reader object and have the openmc or serpent specific readers inherit from it and use the super().__init__() function (see osier.Technology).

ZoeRichter · 2024-05-06T17:22:24Z

Somewhere, this got lost in the shuffle of other things I was doing, and I thought I'd handled this (I haven't). I've read over the comments, and am going to go through either adding changes or responding to comments (I just wanted to make an update). Most of the ones re: longer form names I am up for changing, at the very least.

Sorry this got away from me!

Updates docstrings, variable names, and function names, to improve clarity/readability Co-authored-by: Sam Dotson <[email protected]>

ZoeRichter · 2024-05-13T18:57:22Z

I believe I have incorporated all recommended changes, with the exception of adding the reader parent class (which I'm not strictly opposed to, I mostly want to discuss it first). All changes wrt variable and files names (such as renaming reader to mc_reader) have been committed. I'm going to re-request reviews (thank you both!).

yardasol

Hi @ZoeRichter, Sorry it has taken me so long to get around to this. Great work here, I have a couple suggestions.

src/didymus/mc_reader.py

yardasol · 2024-05-29T16:13:02Z

src/didymus/mc_reader.py

+            for the centroid of a sphere, obtained from the pack_spheres() 
+            function of OpenMC, where N is the number of spheres.


Suggested change

for the centroid of a sphere, obtained from the pack_spheres()

function of OpenMC, where N is the number of spheres.

for the centroid of a sphere, obtained from the :function:`openmc.pack_spheres()`

function, where N is the number of spheres.

I am okay with applying this change, but I'll have to do it manually and change where the line breaks are, because the first one is 81 characters, which (I believe) breaks PEP8 (and I don't want the bot to come yell at me).

src/didymus/mc_reader.py

yardasol · 2024-05-29T16:15:46Z

src/didymus/pebble.py

+    Class for a single pebble object. Contains information
+    necessary to determine the pebble location within the core
+    and track its material composition and unique id number.


Suggested change

Class for a single pebble object. Contains information

necessary to determine the pebble location within the core

and track its material composition and unique id number.

Class representing a TRISO fuel pebble. Contains information

necessary to determine the pebble location within the core

and track its material composition and unique id number.

Definitely think representing is better than for here, but (at least tentatively) there's no reason this could be used with "dummy" graphite-only spheres, so I'm hesitant to specifically call them fuel pebbles. Maybe just "Class representing a pebble"?

src/didymus/pebble.py

Applies all suggested docstring changes from Olek's review, with the exception of two suggestions that either need further discussion or a slight change (see comments under suggestions). Co-authored-by: Olek <[email protected]>

katyhuff · 2024-06-04T15:32:28Z

Looks like these edits have covered the remaining recommendations from the reviewers . Merging!

ZoeRichter added 3 commits February 21, 2024 11:24

pebble class and basic openmc reader

25fcd34

accidentally added cache

970356a

added docstrings

16e3187

ZoeRichter requested review from munkm, samgdotson, yardasol and nsryan2 February 21, 2024 18:30

ZoeRichter self-assigned this Feb 21, 2024

fixing whitespace issues

6230449

nsryan2 approved these changes Feb 21, 2024

View reviewed changes

Commits @nsryan2 suggestions

3e4f1e3

Co-authored-by: Nathan Ryan <[email protected]>

samgdotson requested changes Feb 21, 2024

View reviewed changes

samgdotson requested a review from nsryan2 February 21, 2024 21:25

ZoeRichter and others added 3 commits May 6, 2024 13:13

Apply suggestions from code review

fd4910b

Updates docstrings, variable names, and function names, to improve clarity/readability Co-authored-by: Sam Dotson <[email protected]>

update Pebble call in reader.py

e3da9d2

renamed reader.py to mc_reader.py

15a63ef

ZoeRichter requested a review from samgdotson May 13, 2024 18:57

yardasol requested changes May 29, 2024

View reviewed changes

Apply some suggested docs changes from Olek review

c940e20

Applies all suggested docstring changes from Olek's review, with the exception of two suggestions that either need further discussion or a slight change (see comments under suggestions). Co-authored-by: Olek <[email protected]>

katyhuff added this to the Establish Code Structure milestone May 31, 2024

ZoeRichter added 2 commits May 31, 2024 14:11

manually added some docstring suggestions

e1823b2

Merge branch 'main' of github.com:ZoeRichter/didymus

b48cc8c

fixed trailing whitespacein mc_reader

0cbc31e

katyhuff merged commit 2f6adf3 into arfc:main Jun 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding OpenmcReader and Pebble Classes #1

Adding OpenmcReader and Pebble Classes #1

ZoeRichter commented Feb 21, 2024

pep8speaks commented Feb 21, 2024 •

edited

Loading

nsryan2 left a comment

nsryan2 Feb 21, 2024

samgdotson Feb 21, 2024

ZoeRichter May 7, 2024

nsryan2 Feb 21, 2024

nsryan2 Feb 21, 2024

ZoeRichter May 6, 2024

samgdotson commented Feb 21, 2024

samgdotson left a comment

samgdotson Feb 21, 2024

ZoeRichter May 6, 2024

samgdotson Feb 21, 2024

ZoeRichter commented May 6, 2024

ZoeRichter commented May 13, 2024

yardasol left a comment

yardasol May 29, 2024

ZoeRichter May 29, 2024

yardasol May 29, 2024

ZoeRichter May 29, 2024

katyhuff commented Jun 4, 2024

	Array of center coordinates, in x, y, and z, from
	Array of center coordinates, in x, y, and z from

		for the centroid of a sphere, obtained from the pack_spheres()
		function of OpenMC, where N is the number of spheres.

Adding OpenmcReader and Pebble Classes #1

Adding OpenmcReader and Pebble Classes #1

Conversation

ZoeRichter commented Feb 21, 2024

pep8speaks commented Feb 21, 2024 • edited Loading

Comment last updated at 2024-05-31 19:17:59 UTC

nsryan2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samgdotson commented Feb 21, 2024

samgdotson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZoeRichter commented May 6, 2024

ZoeRichter commented May 13, 2024

yardasol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katyhuff commented Jun 4, 2024

pep8speaks commented Feb 21, 2024 •

edited

Loading