Remove Manual Definition of Position from Cycamore by dean-krueger · Pull Request #641 · cyclus/cycamore

dean-krueger · 2025-06-25T04:59:04Z

Summary of Changes

This PR removes all the manual definition of position in Cycamore Agents.

Related CEPs and Issues

This PR is related to:

Update Position #502
#1853
#1510
Closes #1871
Closes [Task] Clean Manual Position Definitions out of Cycamore Agents #640

Associated Developers

@bam241
@gonuke

Design Notes

I attempted to remove all initialization from the constructor (the latitude(0.0), longitude(0.0), coordinates(0, 0) bit), but because of limitations with the #pragma cyclus var system, it is impossible to inject something like double latitude = 0.0 (it throws an error). I was able to remove the coordinates(0, 0) bit, and to condense the initialization into an injected function, however.

TODO:

Update the changelog (oops)

Testing and Validation

The code was built locally on my machine. No additional tests were added, but all existing tests passed on my machine.

Checklist

Read the Contributing to Cyclus guide.
Compile and run locally.
Add or update tests.
Document if needed.
Follow style guidelines.
Update the changelog.
Address all review comments.
Reviewers, please refer to the Cyclus Guide for Reviewers.

…on and remove the duplucate code

github-actions · 2025-06-25T05:04:40Z

Build Status Report - `2e23bd1` - 2025-07-14 16:00:18 +0000

Build `FROM cyclus_20.04_apt/cyclus:latest`

Cycamore: Success ✅
Cymetric: Success ✅

Build `FROM cyclus_20.04_apt/cyclus:stable`

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

Build `FROM cyclus_20.04_conda/cyclus:latest`

Cycamore: Success ✅
Cymetric: Success ✅

Build `FROM cyclus_20.04_conda/cyclus:stable`

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

Build `FROM cyclus_22.04_apt/cyclus:latest`

Cycamore: Success ✅
Cymetric: Success ✅

Build `FROM cyclus_22.04_apt/cyclus:stable`

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

Build `FROM cyclus_22.04_conda/cyclus:latest`

Cycamore: Success ✅
Cymetric: Success ✅

Build `FROM cyclus_22.04_conda/cyclus:stable`

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

Build `FROM cyclus_24.04_apt/cyclus:latest`

Cycamore: Success ✅
Cymetric: Success ✅

Build `FROM cyclus_24.04_apt/cyclus:stable`

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

Build `FROM cyclus_24.04_conda/cyclus:latest`

Cycamore: Success ✅
Cymetric: Success ✅

Build `FROM cyclus_24.04_conda/cyclus:stable`

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

gonuke

This is definitely looking a lot cleaner. There are a 2 or 3 big design decisions we can discuss in person.

gonuke · 2025-06-25T12:00:01Z

src/deploy_inst.cc

+    latitude(0.0),
+    longitude(0.0) {}


Putting these in the initialization list means that it's a required change for developers to use this toolkit (in addition to the code injection and changes in EnterNotify, for example).

Is there a way to avoid it? The injected code declares a default value of 0 for these. I confess, I can't remember how those defaults are imposed (other than like this), but it would be useful if we could think of a way to avoid changing constructors of agents when we inject code. Maybe that's not realistic. If not, we might want to inject a function that we can call from the constructor rather than messing with initialization lists, so that the developer only has to call the function and doesn't have to know all the details of initialization "under the hood".

I'm not saying there ISN'T, but I will say that I tried a few different things to get rid of this entirely, and ultimately I came back around to having to do it this way. I'm not sure about whether this can be functionalized or not, but your fear from our last FC/S meeting was right on the money. If you don't initialize these variables, you get garbage in them before you call EnterNotify() and initialize them with InitializePosition(). Because of that, if you do something like src_inst = new cycamore::DeployInst(ctx_); (which happens in the tests in the SetUp() function) you get that those variables are full of garbage and specifically the tests fail. Happy to talk more about this in person, and also to try other things, this is just what I settled on last night when I was giving this a first pass.

Let's see if we can safely change the regex at cycpp:L488 to make this work

Another alternative: modify tests to NOT expect these values to be initialized until after the input parameters (XML, DB load) is read/parsed

I went in an changed the throws in the CheckLat and CheckLon functions to cyclus::Warn warnings through cerr so that they don't "throw" but users are still (loudly) notified that they've done something illegal. I had to change a few tests to correctly capture that behavior for testing, but that all happened on the CYCLUS side of things (in case you see something weird and new in there, that's what it's for). This makes it so that all the tests pass, and we can remove the latitude(0.0) longitude(0.0) bits from the initializer lists like we wanted. Felt like a good compromise.

Just to be clear: did NOT have to change cycpp to do this, so we're safe there 😎

gonuke · 2025-06-25T12:01:21Z

src/enrichment.cc

                                    << " entering the simuluation: ";
  LOG(cyclus::LEV_DEBUG2, "EnrFac") << str();
-  RecordPosition();
+  coordinates.RecordPosition(this);


Didn't we record this in EnterNotify? Which raises a tangential question - is there good documentation on the difference between EnterNotify and Build?

It seems like someone put this in Build before because Enrichment didn't implement its own EnterNotify. It should probably only be in one place, and I wonder if this feature is reason enough to implement EnterNotify or can Build be a substitute?

Admittedly, I was making these changes pretty late at night and so I wasn't "thinking" so much as "doing", but you're totally right. Good catch!

gonuke · 2025-06-25T12:05:41Z

src/enrichment.h


 private:
+  // Code Injection:
+  #include "toolkit/position.cycpp.h"


This code injection ends with a protected: block which makes everything after it protected. We should be careful where the code injection happens if it might mess with the protection of member variables and functions

I'll look into this

Should be fixed now

gonuke · 2025-06-25T12:07:30Z

src/growth_region.h

    "\n\n"                                       \
    "  * Start times are inclusive. For a start time :math:`t_0`, the demand "\
-    "function is evaluated on :math:`[t_0, \infty)`."                     \
+    "function is evaluated on :math:`[t_0, infinity)`."                     \


Interesting... I'm pretty sure the previous was chosen to enable LaTeX based rendering.... since this documentation is used in various places, that may be true in the minority of circumstances. Interesting to think this through, though..

I made this change because the compiler was giving a warning about the \i bit, and I was mostly just curious whether this was the thing that was causing that. I forgot that this was used in documentation elsewhere, though. I figured "warning bad", but maybe that's not necessarily true.

gonuke · 2025-06-25T12:09:11Z

src/mixer.cc

+      longitude(0.0) {
  cyclus::Warn<cyclus::EXPERIMENTAL_WARNING>(
      "the Mixer archetype is experimental");
-  RecordPosition();


Yikes - good catch - this should/would probably fail

bam241 · 2025-06-25T12:14:57Z

src/enrichment.h

+  // Code Injection:
+  #include "toolkit/position.cycpp.h"
+


I think an injection there, would have a hidden effect.

As in the injection the last section is protected my guess is that AddMat_() and RecordEnrichment_ will be protected and not private as it was intended.

One need to be careful that injection might change what appear in the parasited file.

Good catch, I've never really understood how the whole protected/public/private thing truly worked, so this flew under my radar. I'll go back and check this all more carefully!

… change I just made in cyclus/cyclus/src/toolkit/position.cycpp.h to remove redundancy in the InitializePosition() function

dean-krueger · 2025-06-27T17:27:01Z

I think this should be "fixed" now, but let me know if either of you see anything else you think should be changed before it's good to go! Thanks a ton for all the review @gonuke @bam241

gonuke

This LGTM - thanks @dean-krueger

gonuke · 2025-07-02T22:08:20Z

This needs a rebase because of the merge of the buy/sell stuff - that should also retrigger tests with the new cyclus code

dean-krueger · 2025-07-04T13:37:42Z

Rebase should have worked, I'll ping you when the tests all pass for a merge

dean-krueger · 2025-07-04T13:41:10Z

Looks like the tests didn't re-trigger? Not totally sure how to get that to happen, but all the checks passed, so I'll lob this back over the wall and hopefully everything is good... @gonuke

… main

… into pos-implementation

dean-krueger · 2025-07-14T16:13:36Z

It looks like the latest branches are all passing now. I think it makes sense that the stable ones are failing, but I'll let you be the judge of that @gonuke

gonuke

This looks right and passes the expected tests. Thanks @dean-krueger

Modified all Cycamore Agents to include the new position code injecti…

f149831

…on and remove the duplucate code

dean-krueger requested review from bam241, gonuke, katyhuff and munkm June 25, 2025 04:59

dean-krueger self-assigned this Jun 25, 2025

gonuke requested changes Jun 25, 2025

View reviewed changes

bam241 reviewed Jun 25, 2025

View reviewed changes

dean-krueger added 3 commits June 25, 2025 11:34

Removed the this from InitializePosition(this) in accordance with the…

c27554e

… change I just made in cyclus/cyclus/src/toolkit/position.cycpp.h to remove redundancy in the InitializePosition() function

Updated Changelog

c7761f3

Removed latitude and longitude from initialization lists in constructors

83f1846

dean-krueger requested a review from gonuke June 27, 2025 17:26

gonuke approved these changes Jun 27, 2025

View reviewed changes

dean-krueger added the Maintenance label Jul 3, 2025

dean-krueger added this to the Position Code Injection milestone Jul 3, 2025

Merge branch 'main' into pos-implementation

0c75e63

dean-krueger added 3 commits July 14, 2025 10:21

removed buy/sell_policy code from storage to get it to play nice with…

81834a2

… main

Merge branch 'pos-implementation' of github.com:dean-krueger/cycamore…

b48fecf

… into pos-implementation

removed duplicate include block

2e23bd1

gonuke approved these changes Jul 15, 2025

View reviewed changes

gonuke merged commit e4ec85c into cyclus:main Jul 15, 2025
14 checks passed

Conversation

dean-krueger commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of Changes

Related CEPs and Issues

Associated Developers

Design Notes

TODO:

Testing and Validation

Checklist

Uh oh!

github-actions bot commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Build Status Report - 2e23bd1 - 2025-07-14 16:00:18 +0000

Build FROM cyclus_20.04_apt/cyclus:latest

Build FROM cyclus_20.04_apt/cyclus:stable

Build FROM cyclus_20.04_conda/cyclus:latest

Build FROM cyclus_20.04_conda/cyclus:stable

Build FROM cyclus_22.04_apt/cyclus:latest

Build FROM cyclus_22.04_apt/cyclus:stable

Build FROM cyclus_22.04_conda/cyclus:latest

Build FROM cyclus_22.04_conda/cyclus:stable

Build FROM cyclus_24.04_apt/cyclus:latest

Build FROM cyclus_24.04_apt/cyclus:stable

Build FROM cyclus_24.04_conda/cyclus:latest

Build FROM cyclus_24.04_conda/cyclus:stable

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!

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!

dean-krueger commented Jun 27, 2025

Uh oh!

gonuke left a comment

Choose a reason for hiding this comment

Uh oh!

gonuke commented Jul 2, 2025

Uh oh!

dean-krueger commented Jul 4, 2025

Uh oh!

dean-krueger commented Jul 4, 2025

Uh oh!

dean-krueger commented Jul 14, 2025

Uh oh!

gonuke left a comment

Choose a reason for hiding this comment

Uh oh!

dean-krueger commented Jun 25, 2025 •

edited

Loading

github-actions bot commented Jun 25, 2025 •

edited

Loading

Build Status Report - `2e23bd1` - 2025-07-14 16:00:18 +0000

Build `FROM cyclus_20.04_apt/cyclus:latest`

Build `FROM cyclus_20.04_apt/cyclus:stable`

Build `FROM cyclus_20.04_conda/cyclus:latest`

Build `FROM cyclus_20.04_conda/cyclus:stable`

Build `FROM cyclus_22.04_apt/cyclus:latest`

Build `FROM cyclus_22.04_apt/cyclus:stable`

Build `FROM cyclus_22.04_conda/cyclus:latest`

Build `FROM cyclus_22.04_conda/cyclus:stable`

Build `FROM cyclus_24.04_apt/cyclus:latest`

Build `FROM cyclus_24.04_apt/cyclus:stable`

Build `FROM cyclus_24.04_conda/cyclus:latest`

Build `FROM cyclus_24.04_conda/cyclus:stable`