Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Source can package #613

Merged
merged 4 commits into from
Jul 18, 2024
Merged

Source can package #613

merged 4 commits into from
Jul 18, 2024

Conversation

nuclearkatie
Copy link
Contributor

Adds the ability for source to provide materials that are packaged.

Along the way, I also converted Source to a ResBuf facility. This means that the full inventory is created at the EnterNotify step and placed into the inventory buffer, then persists throughout the simulation. Previously, material never existed until the moment it was about to be traded, when it would be created. Source just maintained a number that represented the inventory at hand.

This change is generally minor, but it makes packaging easier. Without pre-existing resources in a buffer, trades would be executed by created materials, trying to package them, and then in the rare case where the trade has some leftover material that can't be packaged (rare, and specifically only exists in cases where the requesting agent accepted a partial bid), then you're left with material that was already created but no buffer to place it back into..

This also allows sources to show up on explicit inventory tables, which is something I've wanted for a while anyway, so happy side benefit.

@nuclearkatie nuclearkatie requested a review from gonuke July 16, 2024 19:20
@nuclearkatie nuclearkatie self-assigned this Jul 16, 2024
Copy link

github-actions bot commented Jul 16, 2024

Build Status Report - a9d8f20 - 2024-07-17 20:35:54 -0500

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

@nuclearkatie
Copy link
Contributor Author

Might need to think about whether we want to tamp down on some of the automatic recording that happens during the packaging process. In my testing for this process, each trade package results in this many lines on a resource table:

  1. existing inventory resource
  2. inventory resource minus popped resource
  3. popped resource
  4. popped resource with new composition (in the case that Source has no outrecipe, transformed to requested comp)
  5. package fill amount extracted from popped resource (typically, the full amount)
  6. packaged resource with new package id
  7. inventory with remainder of popped resource pushed back on (in the case that not all of popped resource can fit in package)

I guess these all make sense, but they can be confusing because the amounts are the same. For example when the inventory is large, 1, 2, and 7 can appear identical (i.e. 1e299) even when they're not. When all of the popped resource can be packaged, 3, 4, and 5, and 6 are all the same quantity, with the only difference visible in a resource table being that 6 is finally labeled with the new package.

My point is, a simple transaction has gone from 3 lines in a resource table (initial inventory, popped resource, and new inventory minus popped amount) to 7 lines in a way that might be confusing to users. Things are technically happening to trigger a new record in the resource table, but they're either happening behind the scenes, or they're not really changes at all (especially line 5).

It was confusing to me the developer of packages and packaging in Source, and took me a little while to understand especially steps 3-6

@nuclearkatie nuclearkatie marked this pull request as ready for review July 16, 2024 22:24
@nuclearkatie
Copy link
Contributor Author

cyclus#1775 addresses some of these issues. I also added a line to prevent active transmutation of the resource when there is no request composition or the request composition matches the existing transmutation. Previously, a new line in the resource database would be added if the agent had no outrecipe, even if there was no request composition or the request composition was identical to the dummy composition. This is 4 on the list above, specifically in the case when no changes are occuring

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 questions, some about clarity.

src/source.cc Show resolved Hide resolved
src/source.cc Show resolved Hide resolved
src/source.cc Show resolved Hide resolved
@gonuke gonuke merged commit 2bbf4bc into cyclus:main Jul 18, 2024
10 checks passed
@gonuke
Copy link
Member

gonuke commented Jul 18, 2024

Did I merge too soon?? this is failing on merge even though tests passed on CI.

I see now that it was failing with cyclus:latest and cyclus:stable.... What do we need to fix here? calling @bennibbelink

@bennibbelink
Copy link
Contributor

The failures on cyclus:stable are expected, since "'package' is not a valid XML schema type". We should continue to experience these failures until a new stable version is released.

The failures on cyclus:latest look to be a product of this PR:

#8 1.188 [ RUN      ] SourceTest.Package
#8 1.194 /cycamore/build/cycamore/source_tests.cc:187: Failure
#8 1.194 Expected equality of these values:
#8 1.194   qr_res.rows.size()
#8 1.194     Which is: 6
#8 1.194   3
#8 1.194 
#8 1.196 [  FAILED  ] SourceTest.Package (7 ms)
#8 1.196 [ RUN      ] SourceTest.TransportUnit
#8 1.202 /cycamore/build/cycamore/source_tests.cc:219: Failure
#8 1.202 Expected equality of these values:
#8 1.202   qr_res.rows.size()
#8 1.202     Which is: 8
#8 1.202   4
#8 1.202 
#8 1.204 [  FAILED  ] SourceTest.TransportUnit (8 ms)

@nuclearkatie do you have any insight to these failed tests?

@nuclearkatie
Copy link
Contributor Author

I'll take a look, I'm sure I made a mistake in building those tests somewhere. I was also confused by seeing CI checks passed but the comment showed latest failing

@nuclearkatie
Copy link
Contributor Author

See #617

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants