allowing materials from multiple burnup indexes

[shimwell]

This is an attempt to add support for exporting the materials from multiple burnup indexes.

Currently the depletion Results object has a method called export_to_materials which has a burnup_index argument and the burnup_index argument accepts an int.

This PR modifies the export_to_materials method so that burnup_index can be an int or Iterable of ints or None (in which case it returns all burnup_index)

Here are some example use cases

import openmc
import openmc.deplete

results = openmc.deplete.Results.from_hdf5('depletion_results.h5')

all_mats = results.export_to_materials(burnup_index=None) # returns materials from all burnup indexes
all_mats = results.export_to_materials() # returns materials from all burnup indexes as burnup_index defaults to None
some_mats = results.export_to_materials(burnup_index=[1,2,3])  # returns materials from several burnup indexes
one_mats = results.export_to_materials(burnup_index=[1])  # returns materials from a single burnup indexes
one_mats = results.export_to_materials(burnup_index=1)  # returns materials from a single burnup indexes preserving the current behavior

Sorry for all the line changes, most of the churn is just indentation

Closes #2120

[shimwell]

Thanks for the review and improvements @yardasol I shall get those tests added

[shimwell]

I couldn't find an appropriate unit tests file but added some basic tests to the regression test

[shimwell]

Users could loop over the Results object and achieve the same output. So this is just a small convenience as it avoids users finding the length of the results and allows multiple time steps to be returned at once.

If this gets merged I was going to have a another follow up PR to add a filter that extracted materials that matched a specified material ID. Which would be mainly user convenience again but also speed things up for users who don't want all the materials each time. Longer term I be keen to get materials from time steps by providing the time in seconds/mins/hours/years which is also currently possibe but takes a few extra user lines of code

I thought this function might have filters and be a bit like more the get_tally where we are able to quickly find tallies that match name, score, filter etc

Adding up all these little tweaks up can stack make a decent reduction to the user scripts (see code snippet in PR #2203 for example). So I like to offer them up just in case. Totally understand if this is not needed. Feel free to close

[yardasol]

A design question regarding this — why not just have the user call export_to_materials in a loop if they really want Materials objects for each step?

I personally think it's much nice to just call a function instead of having to write a loop. It's not that writing a loop is hard, it's just really nice to have support for that feature out-of-the-box.

[paulromano]

I kind of have two problems with it as is: 1) the default behavior of generating materials for every single timestep could potentially generate a lot of data and easily blow out someone's memory (imagine you have lots of timesteps and lots of materials), and 2) I'm not a huge fan of having a return type that depends on how the method is called. It becomes hard for a user to reason about code when the return type is not consistent.

see code snippet in PR #2203

We can definitely still add a get_activity method without any of the changes here. Semi-related to this, I'm going to start working on a material composition class that can be used consistently in different places (Material, AtomNumber, StepResult, ...) and should hopefully reduce some of the redundancy across classes.

Totally understand if this is not needed. Feel free to close

Just a reminder that we operate on a consensus-seeking process where the goal is to come to an agreement on what the right path is :smile: I prefer not to make unilateral decisions!

[shimwell]

Let us see if anyone else has a view here, I'm really on the fence. But my gut says Paul is right.

I could make it return a list of lists of materials and default to the last burn-up index if that helps.

I've added a new label added for community feedback requested.

[drewejohnson]

To throw my hat in the ring, an alternative could be to have two methods: one for a single point in time and one for multiple points in time. The former could also rely entirely on the latter with something like

def export_to_materials(self, burnup_index: int) -> Materials:
    return self.export_several_materials([burnup_index])[0]

or the inverse

def export_several_materials(self, burnup_indices: Iterable[int]) -> List[Materials]:
    return [self.export_to_materials(ix) for ix in burnup_indices]

I'm not sold on the method naming tbh, but most of the heavy lifting could be done in one method and shared with the other

If this gets merged I was going to have a another follow up PR to add a filter that extracted materials that matched a specified material ID.

Endorse :+1:

[shimwell]

Nice suggestion Drew 👍 , I shall go ahead and attempt the two method approach and add in the material filter so we can see what this might look like. Also happy to split out the material filter later if that is the only useful part.

[shimwell]

This PR I made doesn't appear to have got the balance of user convenience and developer maintenance burden quite right. I'm going to close this one and made a fresh PR for just the material filtering part that was the most popular part. Feel free to reopen if anyone prefers this approach.