Fix Tests for new Decay on Absorb behavior by dean-krueger · Pull Request #2 · gonuke/cyclus

dean-krueger · 2025-12-04T23:00:46Z

This PR fixes the (/at least) two tests that seemed to be failing in the Cyclus build. The two failing tests were:

MaterialTest.AbsorbPrevDecay and ResourceTest.MaterialUnitValue

MaterialTest.AbsorbPrevDecay was fixed by adding a fake context to the simulation and then stepping through fakectx->time and checking that absorb records the correct timestamp (should be 11)

ResourceTest.MaterialUnitValue was failing because when you made the change in the material.cc Absorb function you accidentally deleted the part that averages the unit value of combined materials. I added that bit back and this test started passing! I guess this just goes to show why tests are so valuable! 😁

Either way, this PR should fix at least those two tests, and they were the ones I saw that were failing in your Cyclus PR, so hopefully this fixes that and then we're good to go!

dean-krueger · 2025-12-04T23:01:17Z

@gonuke Doesn't look like I have permission to add you as a reviewer, so pinging you like this so it (hopefully) shows up in your inbox

gonuke · 2025-12-05T04:50:04Z

Thanks for the fixes here @dean-krueger

All the other decay tests work without a context.

I think we may want to have some tests that just decay these different materials directly (ie. without a context) and then some that rely on the context.

dean-krueger · 2025-12-05T15:45:53Z

I'm not totally clear on what you mean by your comment, but I'll do my best to respond to what I think you mean?

The context is important for this specific test where it may not be for others because the new decay on absorb functionality is fundamentally linked to the time at which the materials are said to have been decayed. Since time lives in the context, we need to have it for this particular test. Since this test is specific to the behavior of materials decay when they are combined with the Absorb function, it seems appropriate to use the context here where is may not be for other tests (though it may also be, I'm not sure I didn't look at other tests, just this failing one).

gonuke · 2025-12-05T16:17:13Z

A couple of related things:

the proposed change allows for abosrbing materials without a context, where the common decay time is simply the max of each of their previous decay times - we should probably test that behavior with materials that are not in a context, in addition to tests that rely on the context time (this was my original point)
(something I realized while reviewing my own PR) the proposed change doesn't compare the current context time with the previous decay times, with the potential for the context time to be less than the previous decay times. This seems:
1. unlikely, and
2. almost certainly a bug if it occurs, but
3. probably OK because decay can be done in reverse?

dean-krueger · 2025-12-05T17:10:07Z

The proposed change allows for abosrbing materials without a context, where the common decay time is simply the max of each of their previous decay times - we should probably test that behavior with materials that are not in a context, in addition to tests that rely on the context time (this was my original point)

A few questions about this:

When you say "the proposed change" are you talking about my fixes to the tests (the subject of this PR) or the change you made to the way the Material::Absorb function works (force both materials to decay on absorption cyclus/cyclus#1918). This question may seem pedantic, but I'm getting a little confused what we're talking about since this is a PR to fix tests into your PR to fix the Absorb function.
Do materials exist in Cyclus simulations without a context? The "Snapshot" materials we use, perhaps (I forget)?. If so, should those materials ever get decayed, since they're "snapshots" and as an extension does that test matter?
If yes to 2, circling back around to 1. is that something that matters for this PR into your PR? (I can see an argument for "we might as well do that since we're doing this", but I might also argue that keeping the scope of these things separate also has value so we don't wind up down a massive rabbit hole of recursively scope-creeped PRs into other PRs)

(something I realized while reviewing my own PR) the proposed change doesn't compare the current context time with the previous decay times, with the potential for the context time to be less than the previous decay times.

Is it possible (or more importantly "indented under normal operation") for Cyclus' context to go backwards in time? I didn't think it was, in which case I agree that this would be a bug, and would then argue that a material absorb test is probably not the place to make sure this isn't happening.

As always happy to do whatever, just wanted to make sure we were on the same page!

gonuke · 2025-12-05T20:52:22Z

Regarding the context:

In the changes that I proposed in my PR force both materials to decay on absorption cyclus/cyclus#1918, there is the possibility for absorb to happen for materials that are not governed by a context.
the previous behavior of Material::Absorb() didn't care about a context because it only picked the decay time of one of the two materials.
I added a check for the context so that we could advance the decay time to the current context time, rather than only the times in the materials.
I can't remember all the ways that we might arrive at materials being outside of a context, but most of the tests for decay throughout the material tests do not assume a context.
if we only want to perform Absorb() on materials in a context, then Material::Absorb() needs to be updated to check that it's true and fail/warn otherwise.
if we want to allow Absorb() for materials not in a context, then we should include tests for that.

gonuke · 2025-12-05T20:54:28Z

Regarding backward decay:

I don't think it's every been considered a normal behavior
if we always want to force decay to move forward we need to not just use the context time, but take the max of the previous decay times and the context time to determine the new time
we can also fail/warn if the previous decay times are ever beyond the current context time
if we leave it as is, it's possible that we cause decay to go backwards
whatever we choose, we need to test it appropriately

dean-krueger · 2025-12-06T15:53:11Z

Gotcha. I'm going to advocate for moving this conversation/issue to the main PR and separating it from this PR, which simply aims to fix those two failing tests. I will make a comment in the main PR about adding additional tests and reference your explanations.

dean-krueger · 2025-12-15T18:29:27Z

Refactor these such that we use the REAL context (not the fake context). @gonuke and I talked about one or two other things maybe but I forget what they were. Perhaps I will remember while I'm fixing the first thing, or maybe he'll leave a comment if he remembers.

dean-krueger · 2026-05-12T14:14:25Z

@gonuke I added the stuff we were talking about (a check to make sure that both materials have a context if we're going to be setting their common decay time to the context time, and a block on backwards decay). This should be ready for another look, and then hopefully we can get this merged so that the other PR (cyclus#1918) can go forward.

gonuke · 2026-05-13T12:46:54Z

I think we need to figure out some rebasing of something somewhere. It's likely that my original PR is now way behind the main branch because this PR into my PR seems to bring a lot of unrelated updates.

dean-krueger · 2026-05-13T14:34:35Z

oh, yeah. It seems so. Does it work if you just sync your branch with main through GitHub? If not I can go through and try to figure out which commits are extra and pull those out.

gonuke · 2026-05-19T16:20:35Z

This may need a rebase against my rebased branch to isolate the differences for the PR

… behavior

…ntext before setting common_decay_time to the context time

dean-krueger · 2026-05-19T17:20:15Z

Rebased and ready for review (I quickly checked that it only has stuff I changed in it now, and I'm PRETTY sure it worked)! @gonuke

github-actions · 2026-05-19T17:40:19Z

Downstream Build Status Report - `fd6c654` - 2026-05-19 12:19:16 -0500

Build `FROM cyclus_20.04_apt/cyclus`

Cycamore: Failure ❌
Cymetric: Skipped due to upstream failure ⚠️

Build `FROM cyclus_20.04_apt/cyclus` --parallel

Cycamore: Failure ❌
Cymetric: Skipped due to upstream failure ⚠️

Build `FROM cyclus_20.04_conda/cyclus`

Cycamore: Failure ❌
Cymetric: Skipped due to upstream failure ⚠️

Build `FROM cyclus_20.04_conda/cyclus` --parallel

Cycamore: Failure ❌
Cymetric: Skipped due to upstream failure ⚠️

Build `FROM cyclus_22.04_apt/cyclus`

Cycamore: Failure ❌
Cymetric: Skipped due to upstream failure ⚠️

Build `FROM cyclus_22.04_apt/cyclus` --parallel

Cycamore: Failure ❌
Cymetric: Skipped due to upstream failure ⚠️

Build `FROM cyclus_22.04_conda/cyclus`

Cycamore: Failure ❌
Cymetric: Skipped due to upstream failure ⚠️

Build `FROM cyclus_22.04_conda/cyclus` --parallel

Cycamore: Failure ❌
Cymetric: Skipped due to upstream failure ⚠️

Build `FROM cyclus_24.04_apt/cyclus`

Cycamore: Failure ❌
Cymetric: Skipped due to upstream failure ⚠️

Build `FROM cyclus_24.04_apt/cyclus` --parallel

Cycamore: Failure ❌
Cymetric: Skipped due to upstream failure ⚠️

Build `FROM cyclus_24.04_conda/cyclus`

Cycamore: Failure ❌
Cymetric: Skipped due to upstream failure ⚠️

Build `FROM cyclus_24.04_conda/cyclus` --parallel

Cycamore: Failure ❌
Cymetric: Skipped due to upstream failure ⚠️

dean-krueger · 2026-05-28T21:52:44Z

Wanted to ping this again @gonuke because this is currently my highest-priority task in the v1.7 release project and I think it should be pretty close to ready.

gonuke

It seems we should maybe document/discuss the different states of things for this

gonuke · 2026-05-19T16:18:16Z

@@ -106,8 +106,8 @@ void Material::Absorb(Material::Ptr mat) {

  // force both mateiral objects to advance to the same decay time


Suggested change

// force both mateiral objects to advance to the same decay time

// force both material objects to advance to the same decay time

Yep, whoops!

gonuke · 2026-05-28T22:03:37Z

  int common_decay_time = std::max(this->prev_decay_time_, mat->prev_decay_time_);
-  if (ctx_ != NULL && ctx_->sim_info().decay != "never") {
-    common_decay_time = ctx_->time();    
+  if (HasContext() && mat->HasContext() && ctx_->sim_info().decay != "never") {


I'm trying to understand why we care if the material has a context here?

Maybe I misinterpreted something, but didn't we have a whole conversation about these needing a context above?

gonuke · 2026-05-28T22:07:55Z

+  // Block backwards decay with std::max
+  int dt = std::max(curr_time - prev_decay_time_, 0);


Is this change necessary to fix the problems we're experiencing?

Is it possible for this to occur? and might there be cases where we want to see where a material came from?

Again, I think you suggested we do this?

Regarding backward decay:

I don't think it's every been considered a normal behavior

if we always want to force decay to move forward we need to not just use the context time, but take the max of >the previous decay times and the context time to determine the new time

we can also fail/warn if the previous decay times are ever beyond the current context time

if we leave it as is, it's possible that we cause decay to go backwards

whatever we choose, we need to test it appropriately

gonuke · 2026-05-28T22:17:39Z

+  if (HasContext() && mat->HasContext() && ctx_->sim_info().decay != "never") {
+    common_decay_time = std::max(ctx_->time(), common_decay_time);    
  }


I'm not sure I agree with this choice. I guess we haven't thought through all the possible permutations of the state of each material and what we should do in those states...

gonuke · 2026-05-28T22:21:02Z

  m1->Absorb(m3);
-  EXPECT_EQ(10, m1->prev_decay_time());
+  EXPECT_EQ(11, m1->prev_decay_time());
  m1->Absorb(m2);
-  EXPECT_EQ(10, m1->prev_decay_time());
+  EXPECT_EQ(11, m1->prev_decay_time());


I wonder if we should test that decay actually occurs? Or do we consider it covered by other tests?

We test the actual decay behavior in a few places, typically by calling decay and then checking that it's done something (using the EXPECT_NE test) but specifically in DecayCustomTimeStep we check that 1/2 of Cs-137 decays in one halflife of Cs-137, so I'd consider that "tested"

gonuke · 2026-05-28T22:28:08Z

Let me put some thoughts here on relative decay times of materials. To begin with, let's assume that the context for the two materials is the same. If that's not true, we're in a very different space...

ctx->time	mat1->prev_decay_time	mat2->prev_decay_time	target decay time
t	<= t	<= t	t
t	<= t	> t	?
t	> t	<= t	?
t	> t	> t	?

In practice, it's not clear how we get into a state that is in rows 2-4 here? Do we need to allow for them? Do we throw an error?

gonuke · 2026-05-29T02:17:11Z

Apologies - I did not go back and read the full chain of discussion and will need to do that and update my comments/questions!

dean-krueger · 2026-05-29T02:21:38Z

No worries! I just wanted to make sure we were on the same page!

gonuke · 2026-05-29T02:51:31Z

(We'll probably want to capture this discussione somewhere else, eventually!!)

I looked at all the ways that prev_decay_time_ can ever get changed.

when a material is extracted (extracted to a package), it's prev_decay_time_ is set to match its parent
(historically) when a material is absorbed, the prev_decay_time_ is set to match the larger of the two materials (this was always pretty questionable)
when a material is transmuted we always set the prev_decay_time_ to the context's current time, if the material exists within a context
it is initialized to 0 and then set to the current context time on construction, if it has a context
following decay it is set to match the decay time (which can be arbitrarily far in the past/future)

It's thus probably useful to understand the range of outcomes from Materaial::Decay():

If there is no context for the material, the curr_time must be specified when calling Decay() but can be any value relative to prev_decay_time_, including an earlier time.
If there is a context, the curr_time can be specified an be any value relative to prev_decay_time_
If there is a context, the default value is the context's current time, which may be any value relative to prev_decay_time_ depending on how prev_decay_time_ has been determined

I think it makes sense for untracked materials (i.e. those without a context) to be allowed to decay forward or backward in time.

A key discussion point: Is there a use case for allowing a material that is part of a context to decay backwards in time or beyond the current context time?

My first instinct is to say that all materials with a context should always decay to the current context time and only the current context time. The consequences of this:

If a user wanted to probe the composition of a material that decayed forward or backward to some other time, they could clone it to an untracked material and then have that material decay arbitrarily.
All absorption should only be between materials that are either
- both outside any context and everyone needs to be careful, OR
- both in the same context and therefore constrained in their prev_decay_time_

What should the behavior be for absorbing two materials outside any context? We can choose an arbitrary decay time for these after the absorption is complete. Since we have no context, it needs to be based on the prev_decay_time_ of the two materials, and perhaps we can just choose the largest of those?

gonuke force-pushed the decay_absorb branch from 04e4f51 to d9bef8f Compare May 19, 2026 16:15

dean-krueger added 3 commits May 19, 2026 12:18

changed the AbsorbPrevDecay test from material_tests to fit new decay…

1a0fa7b

… behavior

added unitvalue averaging back to absorb to restore tested behavior

5163ac3

added blocks for backwards decay, and checks for both matls having co…

fd6c654

…ntext before setting common_decay_time to the context time

dean-krueger force-pushed the decay-absorb branch from 9d75ffb to fd6c654 Compare May 19, 2026 17:19

gonuke requested changes May 28, 2026

View reviewed changes

		@@ -106,8 +106,8 @@ void Material::Absorb(Material::Ptr mat) {

		// force both mateiral objects to advance to the same decay time

	// force both mateiral objects to advance to the same decay time
	// force both material objects to advance to the same decay time

		// Block backwards decay with std::max
		int dt = std::max(curr_time - prev_decay_time_, 0);

Conversation

dean-krueger commented Dec 4, 2025

Uh oh!

dean-krueger commented Dec 4, 2025

Uh oh!

gonuke commented Dec 5, 2025

Uh oh!

dean-krueger commented Dec 5, 2025

Uh oh!

gonuke commented Dec 5, 2025

Uh oh!

dean-krueger commented Dec 5, 2025

Uh oh!

gonuke commented Dec 5, 2025

Uh oh!

gonuke commented Dec 5, 2025

Uh oh!

dean-krueger commented Dec 6, 2025

Uh oh!

dean-krueger commented Dec 15, 2025

Uh oh!

dean-krueger commented May 12, 2026

Uh oh!

gonuke commented May 13, 2026

Uh oh!

dean-krueger commented May 13, 2026

Uh oh!

gonuke commented May 19, 2026

Uh oh!

dean-krueger commented May 19, 2026

Uh oh!

github-actions Bot commented May 19, 2026

Downstream Build Status Report - fd6c654 - 2026-05-19 12:19:16 -0500

Build FROM cyclus_20.04_apt/cyclus

Build FROM cyclus_20.04_apt/cyclus --parallel

Build FROM cyclus_20.04_conda/cyclus

Build FROM cyclus_20.04_conda/cyclus --parallel

Build FROM cyclus_22.04_apt/cyclus

Build FROM cyclus_22.04_apt/cyclus --parallel

Build FROM cyclus_22.04_conda/cyclus

Build FROM cyclus_22.04_conda/cyclus --parallel

Build FROM cyclus_24.04_apt/cyclus

Build FROM cyclus_24.04_apt/cyclus --parallel

Build FROM cyclus_24.04_conda/cyclus

Build FROM cyclus_24.04_conda/cyclus --parallel

Uh oh!

dean-krueger commented May 28, 2026

Uh oh!

gonuke 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!

gonuke commented May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gonuke commented May 29, 2026

Uh oh!

dean-krueger commented May 29, 2026

Uh oh!

gonuke commented May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Downstream Build Status Report - `fd6c654` - 2026-05-19 12:19:16 -0500

Build `FROM cyclus_20.04_apt/cyclus`

Build `FROM cyclus_20.04_apt/cyclus` --parallel

Build `FROM cyclus_20.04_conda/cyclus`

Build `FROM cyclus_20.04_conda/cyclus` --parallel

Build `FROM cyclus_22.04_apt/cyclus`

Build `FROM cyclus_22.04_apt/cyclus` --parallel

Build `FROM cyclus_22.04_conda/cyclus`

Build `FROM cyclus_22.04_conda/cyclus` --parallel

Build `FROM cyclus_24.04_apt/cyclus`

Build `FROM cyclus_24.04_apt/cyclus` --parallel

Build `FROM cyclus_24.04_conda/cyclus`

Build `FROM cyclus_24.04_conda/cyclus` --parallel

gonuke commented May 28, 2026 •

edited

Loading

gonuke commented May 29, 2026 •

edited

Loading