Skip to content

Conversation

@dean-krueger
Copy link
Contributor

@dean-krueger dean-krueger commented Aug 6, 2025

Summary of Changes

I feel like I've seen this suggestion for a region pop up often enough, especially in my conversations with @gonuke that I figured it'd be fun to just go ahead and make one.

This is TariffRegion. It allows users to specify "friend" regions and "enemy" regions within the simulation and then impose tariffs on the enemies, and subsidies on the friends. The subsidies and tariffs are applied to all trades coming from those regions, so this is fairly basic (it'd be nice maybe later to make it so that only specific commodities from those regions got affected, but I threw this together last night and this afternoon, so I didn't get that far).

Related CEPs and Issues

This PR is related to:

Associated Developers

Cursor AI

Design Notes

Believe it or not, I did most of this on my own. Cursor helped a bit here and there, obviously, but mostly with me understanding how things worked (and the occasional quick "hit tab to fill in the rest of this in that similar function" bits). The basics of this were based on the tutorial PR version of TariffRegion, so I guess in that sense Cursor did a bit more of the heavy lifting.

Testing and Validation

All tests pass. Cyclus and Cycamore both build, and both sets of tests pass with these changes. Unit tests have NOT been added for this Agent.

Checklist

@dean-krueger dean-krueger requested review from gonuke and munkm August 6, 2025 19:45
@dean-krueger dean-krueger self-assigned this Aug 6, 2025
@github-actions
Copy link

github-actions bot commented Aug 6, 2025

Build Status Report - 72db1b8 - 2025-12-04 11:26:53 -0600

Build FROM cyclus_20.04_apt/cyclus:latest
  • Cycamore: Success
  • Cymetric: Failure
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: Failure
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: Failure
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: Failure
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: Failure
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: Failure
Build FROM cyclus_24.04_conda/cyclus:stable
  • Cycamore: Failure
  • Cymetric: Skipped due to upstream failure ⚠️

@dean-krueger
Copy link
Contributor Author

Here are some pictures of what the TariffRegion does (in conjunction with cyclus/cyclus#1897 and a hack I made for testing where sources bid in at a cost of 10.0)

Screenshot 2025-08-06 at 2 10 50 PM Screenshot 2025-08-06 at 2 11 24 PM Screenshot 2025-08-06 at 2 12 07 PM Screenshot 2025-08-06 at 2 12 18 PM

Notice that for subsidies greater than 100% (1.0) the cost goes to 0, and not something negative or nan-like. Ignore the country names and which ones get tariffs and subsidies, I sort of just picked countries as they came to mind and then semi-randomly made them friends/enemies.

@gonuke
Copy link
Member

gonuke commented Aug 7, 2025

Saying a lot about who your friends/enemies are 🤣

But seriously....

What if we didn't need to specify as friends vs enemies, and just had one list of regions and one set of multipliers that are either positive or negative depending on whether it's a tariff or subsidy?

@gonuke
Copy link
Member

gonuke commented Aug 7, 2025

You said all test pass, but there are no new tests and these don't impact existing tests.

@dean-krueger
Copy link
Contributor Author

Oh, oops. I didn't realize I said that twice. I meant that this doesn't break any other cyclus or cycamore tests, which I guess it shouldn't because it's its own thing, so you make a good point. As for your point about just a list of regions to adjust with a corresponding list of adjustments, that's not a bad idea. I'm a little too fond of organizing things into little boxes, so in my head it feels nicer to have them in the "appropriate place", but I guess they don't need to be, and that would probably simplify things.

@dean-krueger
Copy link
Contributor Author

dean-krueger commented Aug 10, 2025

Okay, the whole friend/enemy thing has been removed and now you just add "adjustments" which are tariffs/subsidies through positive or negative values in the adjustment list. Tried to make the documentation clear that it was going to be 1 + whatever the value was (ie a 25% tariff would be 0.25 and adjust the value by x1.25, and a 12.5% subsidy would be -0.125). Here's a screen grab of the new region block, and some output showing it working (note that "France" and "Germany" are still in the simulation, but since they don't appear in the region list of Tariff Region they don't get adjusted).

Screenshot 2025-08-10 at 9 57 08 AM Screenshot 2025-08-10 at 9 57 47 AM

I'm holding off on adding tests until cyclus/cyclus#1897 gets merged, since without that there isn't a really good way to test this region (unless you can think of one).

@gonuke
Copy link
Member

gonuke commented Aug 10, 2025

Can't you test by checking the values of adjusted trades in memory, I.e. before/independent from writing to the DB?

Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting to think through the best nesting of various loops in the adjustment process


#pragma cyclus var { \
"default": [], \
"doc": "Adjustments (1+val) to apply to trades from affected " \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"doc": "Adjustments (1+val) to apply to trades from affected " \
"doc": "Adjustments (multiply by (1+val)) to apply to cost of trades from affected " \

Comment on lines 27 to 36
// Traverse up the hierarchy to get the supplier's region
cyclus::Region* supplier_region = nullptr;
cyclus::Agent* current = supplier;
while (current != nullptr) {
supplier_region = dynamic_cast<cyclus::Region*>(current);
if (supplier_region != nullptr) {
break; // Found a region
}
current = current->parent();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a useful functionality to have somewhere in Cyclus. In most cases, this should just be parent()->parent(), but your approach allows for more complex hierarchies in the future, e.g. subfacilities, etc.

See Agent::AncestorOf(), AgentDescendentOf(), and Agent::InFamilyTree()...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I THINK Agent::AncestorOf(), etc would be really nice if we could somehow just jump to the Region, but because (at least I don't think) you can't access the list of all agents (it's a private member that RIF's don't have access to), it's hard to do something like is "this Agent" an ancestor of "that Agent". At least in a Region. I agree, though, that those functions you mentioned below (FindRegionByName) would be a nice function in Cyclus, where I think it would be possible to do something like that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's try implementing some kind of GetRegion() (and GetInstitution()) for each Facility

Comment on lines 21 to 22
cyclus::Region* FindRegionByName(const std::string& name);
cyclus::Region* FindRegionInHierarchy(cyclus::Agent* agent, const std::string& name);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see these implemented anywhere... surprised that it builds....

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are probably better implemented in cyclus/cyclus anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, they were leftovers from an old implementation route I was going down before I decided to switch gears. I got rid of them (though I do think a FindAgentByName(std::string name) function in Cyclus/Cyclus would be pretty nice...)

@dean-krueger
Copy link
Contributor Author

Can't you test by checking the values of adjusted trades in memory, I.e. before/independent from writing to the DB?

Probably, yeah. I was holding back on doing this partially because it was less clear how to do, and partially because it's not (so far as I know) how we "typically" write these tests (or at least not how I've typically approached them). If you'd like me to try going down that road, though, I'd be happy to do so!

@dean-krueger
Copy link
Contributor Author

okay, I gave this a close read and I think I caught all the stuff we usually talk about in these reviews. Trying to be better/more careful with what I submit so it's a little easier to review now that I have some "commonly identified suggestions" to think about. Can't promise I've gotten them all, but I think it should be pretty close!

@dean-krueger dean-krueger requested a review from gonuke August 19, 2025 15:22
Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more comments here on some cyclus infrastructure help

Comment on lines 49 to 50
double cost_multiplier = 1.0 + adjustments[it - region_names.begin()];
double pref = 1.0 / cost_multiplier;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this honor the existing preference before adjustment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe so, but I should have called it "pref_multiplier", since three lines later we do bid_pair.second *= pref_multiplier, where bid_pair.second is the preference of the bid. I'll fix this.

cyclus::Region* supplier_region = nullptr;
cyclus::Agent* current = supplier;
while (current != nullptr) {
supplier_region = dynamic_cast<cyclus::Region*>(current);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check Agent->kind() for "Region"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some poking into this, and my new understanding is that if we were to do something like if (current->kind() == "Region") { we'd need to then dynamic_cast to region anyway to return the pointer to the region anyway (which is what we want in the end), so checking that way would be redundant. I'm working on converting this to a facility function and then using that in tariff_region now, so maybe it'll make sense in that context?

Comment on lines 27 to 36
// Traverse up the hierarchy to get the supplier's region
cyclus::Region* supplier_region = nullptr;
cyclus::Agent* current = supplier;
while (current != nullptr) {
supplier_region = dynamic_cast<cyclus::Region*>(current);
if (supplier_region != nullptr) {
break; // Found a region
}
current = current->parent();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's try implementing some kind of GetRegion() (and GetInstitution()) for each Facility

@dean-krueger
Copy link
Contributor Author

I feel very strong negative emotions about the data types available on our back end. I keep trying to do something clever and then I just can't? I'm going to address the comments in this PR review without it for now, but mark my words I will be coming back to the back-end.

@dean-krueger
Copy link
Contributor Author

dean-krueger commented Nov 12, 2025

Might need to consider all the possible permutations now that the complexity of this has increased.

In general, we have a list of rules:

  • supplier_region
  • commodity
  • adjustment

For the flat adjustments as implemented now, I have recommended we just implement that as a rule with commodity == "".

I will address this now

There are two permutations not yet implemented:

  • an adjustment for some_commodity to be applied to all regions - perhaps with supplier_region == ""?
  • a flat adjustment to be applied to all commodities from all regions, perhaps with supplier_region == commodity == ""?>

Both of these would require additional inputs

I am going to hold off on this because the input file structure for this is making me feel physically ill to look at, and at this point I am very certain I want to wrestle with (later) the back end, so it'd be a bummer to implement this now and then just redo it later.

@dean-krueger
Copy link
Contributor Author

okay, I went through these comments and fixed the ones where it felt like it was worth doing it now (in case I don't mess with the back end and restructure this), but I left some of them as is because it is my understanding that they're "technically" working and if I did a restructure they'd just be completely irrelevant. We can/should talk about this during the FCS meeting tomorrow, and I am intentionally not re-requesting review because of that.

Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more tweaks while you think about backends

}

void TariffRegion::Tock() {
RecordTariffConfiguration();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to do this every Tock()? It doesn't change over time, does it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! I wonder if I can move that to EnterNotify(), and if not I'll just add an if (!ConfigurationRecorded()) {} catch to avoid doing it every time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason it couldn't be done in EnterNotify?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And if not EnterNotify then slightly better in the Tick before the DRE uses it

Comment on lines 59 to 83
// Template function to reduce code duplication between AdjustMatlPrefs and
// AdjustProductPrefs
template<typename T>
void AdjustPrefsImpl(typename cyclus::PrefMap<T>::type& prefs) {
for (auto& req_pair : prefs) {
std::string commodity = req_pair.first->commodity();

for (auto& bid_pair : req_pair.second) {
cyclus::Bid<T>* bid = bid_pair.first;
cyclus::Facility* supplier = dynamic_cast<cyclus::Facility*>(bid->bidder()->manager());

// Find if any of the supplier's parent regions match our tariff list
cyclus::Region* matching_region = FindMatchingRegion(supplier);
if (matching_region) {
double adjustment = FindTariffForCommodity(matching_region, commodity);

double cost_multiplier = 1.0 + adjustment;
double pref_multiplier = 1.0 / cost_multiplier;
double inf = std::numeric_limits<double>::infinity();

bid_pair.second *= cost_multiplier > 0.0 ? pref_multiplier : inf;
}
}
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These often live outside the the class declaration, at the end of this file, appropriately scoped, i.e. void TariffRegion::AdjustPrefsImpl(...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know that, but glad I do now! I'll look into doing it this way.

@dean-krueger
Copy link
Contributor Author

dean-krueger commented Nov 17, 2025

⚠️ NOTE: THIS PR STILL LACKS TESTS ⚠️

Should be ready for another round of review! In conjunction with cyclus/cyclus#1922 we now have a new datatype on the backend of cyclus (somehow that was the easy part of all this), and now TariffRegion has been completely refactored to use that instead of all those horrid lists. This should hopefully make it FAR easier to parse the code (and input files) and understand what's going on.

I've also included an example XML input file in the input folder (which I figured was probably for that sort of thing) which both shows off the new input structure, and also provides users with an example of how to use a TariffRegion. I would love if @nsryan2 and Taesuk (whose GitHub ID I don't know but who I'll ping on slack) would take a look at that and maybe take it for a spin if they have time (but also I know you guys are busy so no worries if you can't).

Finally, I've attached a few pictures of the tables this Region generates in the output:

Blanket Tariffs (Tariffs on all goods from each named region):
Screenshot 2025-11-17 at 7 57 11 PM

Commodity Specific Tariffs (Tariffs on specific commodities from specific regions, the "normal" kind):
Screenshot 2025-11-17 at 7 58 11 PM

Global Blanket Tariff (A tariff on all goods coming from all regions into the TariffRegion):
Screenshot 2025-11-17 at 8 03 38 PM

Global Commodity Tariff (Tariffs on certain commodities regardless of where they come from):
Screenshot 2025-11-17 at 8 04 19 PM

(Just to get ahead of it, combining these tables would probably be a massive pain, but idk maybe not)

It's worth noting that TariffRegion uses an "overriding" tariff system. That is: tariffs with higher priority override tariffs with lower priority, they do not add. The priority list is recorded in the code as a comment, but I'll list it here as well:

Tiered override system (most specific to least specific):

  1. Region-specific commodity adjustment (highest priority)
  2. Region blanket adjustment
  3. Global commodity adjustment
  4. Global blanket adjustment (lowest priority)

I highly recommend that reviewers glance at the input file structure first and then if they approve of that mark it as viewed or something since that accounts for ~350/600 lines of code added in this PR.

Hopefully this Region is what everyone was looking for in spirit! It's been a lot of fun to develop, and I think will be equally fun/interesting to use with some of the other stuff I've got coming down the pipe.

@dean-krueger dean-krueger requested a review from gonuke November 18, 2025 02:12
@dean-krueger
Copy link
Contributor Author

Needs a slight refactor after the modifications made to cyclus/cyclus#1922. Cannot use empty maps any longer. Must populate commodities with "dummy" empty ones (or real ones with a 0% adjustment) when not imposing commodity-based tariffs.

@dean-krueger
Copy link
Contributor Author

Just occurred to me that being able to specify a time at which tariffs become active (with a default like -1 signifying "from the beginning of the simulation) would be REALLY nice for each commodity. Not sure if that involves adding another back-end datatype, but if it does what a wonderful opportunity that would be to test the tutorial...

@dean-krueger
Copy link
Contributor Author

It will be important to, at some point (this is a bit of a chicken and egg issue I'm realizing), to adjust the implementation of the actual "tariff" part of this facility to match the "Welfare Economics" model we're hoping to use in Cyclus. Right now it assumes the weight-function is always 1/pref, and then updates the PREFERENCES during the preference adjustment phase of the DRE, but we may want to start thinking of that phase more as a a arc weight adjustment phase and using agents like this to modify either MC or MU or some other method of modifying the arc weight directly (typically modifying MC or MU should modify the "cost")

@gonuke
Copy link
Member

gonuke commented Nov 24, 2025

Just occurred to me that being able to specify a time at which tariffs become active (with a default like -1 signifying "from the beginning of the simulation) would be REALLY nice for each commodity. Not sure if that involves adding another back-end datatype, but if it does what a wonderful opportunity that would be to test the tutorial...

I've resisted the urge to bring up time-varying tariffs... I think the input becomes horrific for that...

Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's getting close... a couple of lingering issues

return region->prototype();
}

cyclus::Region* TariffRegion::FindMatchingRegion(cyclus::Facility* supplier) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this will only ever return the first parent region that matches. Multiple parent regions is currently a rare edge case, but this function is written for the existence of multiple parent regions.

What behavior do we want?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh interesting, I did't think of that. I will consult my Tax Law friend and see what he says, but it seems like we have the following problem/options:

Imagine I am Madison, and I want to order a good from my hometown of Webster Groves, which is in St Louis, which is in Missouri. The following tariff structure exists between Madison and all those regions:

Webster Groves: 1% on all goods
St. Louis: 10% on all goods
Missouri: 5% on all goods

Would I want to aggregate the tariffs and impose a 16% tariff on a good coming from Webster groves, take the highest tariff from the nested structure (ie 10%), take the “most superior” tariff (ie the 5% from Missouri), or the most “literal” tariff: the 1% from Missouri? Furthermore, if those are the rules for Madison, but Wisconsin has its own tariff relationship with those regions, since Madison is a sub-region of Wisconsin, how does that relationship work?

I will get back with what his thoughts are, and we can discuss. Fun question!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will consult my Tax Law friend and see what he says

After a lengthy parenthetical about why "Tariff" is a bad word to use for these due to the unconstitutionality of interstate tariffs, he basically said "It depends a lot on the actual wording of the source of the 'tariff', at which point you could basically get any behavior you wanted, so it might be best to just pick a behavior, state it clearly so everyone knows what it's modeling, and then if someone else wants different behavior tell them they can change/add it if they want".

I'm going to do some more poking around to see if I can't find a good justification for one type of behavior over another, but probably just picking something, making it clear what we're doing, and sticking with it will be the best route forward.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, after some thought here's what I think makes the most sense:

Aggregate adjustments from importers along hierarchy of trade below the "Lowest Common Ancestor" of both regions you're trading between for the most specific rule they have to apply. For example, imagine a hierarchy that looks like this:

Mars
---Federation of Independent Martian Colonies
------Zixzorp
Earth
---USA
------Wisconsin
---------Madison
------Missouri
---------Saint Louis
------------Webster Groves
------Illinois
---------Chicago
---Canada
------Ontario
---------Toronto
------Alberta
---------Calgary

Each region and sub region is allowed to be a TariffRegion, and thus have its own rules about each other region in the simulation. Imagine, then a trade between Webster and Madison. What I'm proposing is that we:

  1. take our two regions, compute their Lowest Common Ancestor (LCA), which in this case would be the USA region
  2. compute the chain of travel, which here would be Webster --> Saint Louis --> Missouri --> Wisconsin --> Madison
  3. For all TariffRegions on the importer side (Madison and Wisconsin)
    a. Find the most specific tariff to apply
    b. Add all those up

So for instance, if Madison had these rules:

Webster Groves 1%
St. Louis 10%
Missouri 5%
United States 0%
Rest of World 12%

and Wisconsin had these rules:

Missouri 7%
United States 1%
Rest of World 8%

We would see Madison has a specific rule about Webster, so it gets 1% from Webster --> Madison
We would see Wisconsin has no Webster specific rule --> check parent: no specific saint Louis rule --> Check parent: a specific rule about Missouri so it gets 7% from Missouri --> Wisconsin

And the total tariff to apply is 1% + 7% = 8%

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there is probably no single right answer here, I'm happy with any choice that makes sense and is well documented.

Comment on lines +81 to +85
// Tier 2: Use region blanket adjustment
double region_blanket = region_data.first;
if (region_blanket != 0.0) {
return region_blanket; // Override global adjustments
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to exempt a region from the global adjustment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doesn't sound very much like a global adjustment...

But I see your point. No is the answer, and I am tempted to say we shouldn't add that, but if you feel strongly about it I can.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to leave this for now. Can you make an issues for this as a feature request?

// clang-format off
#pragma cyclus var { \
"default": {}, \
"alias": ["adjustment_regions", "region", ["RegionConfig", "blanket_adjustment", ["commodity_adjustments", "commodity", "adjustment"]]], \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommend calling this "TariffConfig" instead

Suggested change
"alias": ["adjustment_regions", "region", ["RegionConfig", "blanket_adjustment", ["commodity_adjustments", "commodity", "adjustment"]]], \
"alias": ["adjustment_regions", "region", ["TariffConfig", "blanket_adjustment", ["commodity_adjustments", "commodity", "adjustment"]]], \

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can do that! I'll make that change with a few other ones on my end and then push (for some reason the "commit change" on GitHub option scares me)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be done now!

…s in the hierarchy and the behavior should be as described in my comment on the subject
@dean-krueger
Copy link
Contributor Author

⚠️ NOTE: THIS PR STILL LACKS TESTS ⚠️

I think the time-varying tariffs needs to be kicked down the road, but is something that should be added later for sure (I can make an issue if you agree), and the "global-but-not-quite-because-there-are-exceptions" adjustments I think is maybe not the best idea. If you feel strongly about either of these let me know. Otherwise I think this is good for another round of review!

@dean-krueger dean-krueger requested a review from gonuke December 4, 2025 17:29
Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more thoughts and concerns about how this actually functions. I worry that the scope has crept by considering complicated region ancestries that are, frankly, not that common.

}

void TariffRegion::Tock() {
RecordTariffConfiguration();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And if not EnterNotify then slightly better in the Tick before the DRE uses it

}
}

// No common ancestor found (shouldn't happen in a well-formed simulation)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like it will be quite common. In the simplest case that has more than one region, there would be two regions with no relationship.

auto region_it = tariff_importer->adjustment_regions_.find(supplier_ancestor_name);
if (region_it != tariff_importer->adjustment_regions_.end()) {
// Found a match! Now apply the tiered override system for this specific region
return tariff_importer->FindTariffForCommodity(supplier_ancestor, commodity);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is no rule for this (supplier_region, commodity) pair, what does this return? and do we miss the change to apply a global commodity rule below? I see that the global commodity rule is checked in FindTariffForCommodity)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does seem like you miss the chance to find a commodity-specific adjustment in an ancestor of this supplier_region... but maybe that's a case you've explicitly decided to avoid.

Comment on lines +164 to +165
auto region_it = adjustment_regions_.find(region_name);
if (region_it != adjustment_regions_.end()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the place where we call this, we've just done this search - can we reuse that info somehow?

// Get supplier's full ancestor chain (for "most specific" lookups)
std::vector<cyclus::Region*> supplier_hierarchy = GetAncestorChain(supplier_region);

// Get importer chain: from requester up to (but not including) LCA
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you added an optional variable to GetAncestorChain could you use it to get this truncated chain?
importer_chain = GetAncestorChain(requester_region, lca);

Comment on lines +78 to +89
#pragma cyclus var { \
"default": 0.0, \
"doc": "Optional global blanket adjustment applied to all regions and commodities." \
}
double global_blanket_adjustment;

#pragma cyclus var { \
"default": {}, \
"alias": ["global_commodity_adjustments", "commodity", "adjustment"], \
"doc": "Optional global commodity adjustments applied to all regions for specific commodities." \
}
std::map<std::string, double> global_commodity_adjustments_;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if these just lived in your normal adjustment_regions_ list, but we used "*" for global region and "*" for global commodities. Then you wouldn't need to worry about empty lists. You could still extract and store that data in useful data structures after reading the XML (maybe in EnterNotify())?

@dean-krueger
Copy link
Contributor Author

Unless this comment gets updated, it seems like we decided to axe the whole "super region import tariff structure" thing since currently nobody uses that, which I think also resolves most of these other comments (except maybe the location of the record function). I'll work on doing that (probably tomorrow).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature] Add blanket tarrif/subsidy capability [Feature] Cycamore Could Use More Regions

4 participants