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

Add packaging to matl sell policy #1749

Merged
merged 28 commits into from
May 30, 2024
Merged

Add packaging to matl sell policy #1749

merged 28 commits into from
May 30, 2024

Conversation

nuclearkatie
Copy link
Contributor

@nuclearkatie nuclearkatie commented May 13, 2024

Material sell policy can be initialized with a package type. When responding to requests, checks against packaging restrictions. When returning trades, packages material popped from buffer.

@nuclearkatie nuclearkatie self-assigned this May 13, 2024
Copy link

github-actions bot commented May 13, 2024

Downstream Build Status Report - 6144905 - 2024-05-28 08:59:12 -0600

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

@coveralls
Copy link
Collaborator

coveralls commented May 13, 2024

Pull Request Test Coverage Report for Build 9271705192

Details

  • 182 of 197 (92.39%) changed or added relevant lines in 16 files are covered.
  • 2 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.06%) to 40.305%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/context.cc 10 11 90.91%
src/product.cc 11 12 91.67%
src/sim_init.cc 13 14 92.86%
src/resource.h 3 5 60.0%
src/toolkit/matl_sell_policy.cc 23 33 69.7%
Files with Coverage Reduction New Missed Lines %
src/resource.h 1 73.91%
src/product.cc 1 80.0%
Totals Coverage Status
Change from base Build 9141541465: 0.06%
Covered Lines: 52205
Relevant Lines: 129526

💛 - Coveralls

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.

Some first thoughts...

src/toolkit/matl_sell_policy.h Outdated Show resolved Hide resolved
src/toolkit/matl_sell_policy.cc Outdated Show resolved Hide resolved
src/toolkit/matl_sell_policy.cc Outdated Show resolved Hide resolved
src/toolkit/matl_sell_policy.cc Outdated Show resolved Hide resolved
src/toolkit/matl_sell_policy.cc Outdated Show resolved Hide resolved
src/toolkit/matl_sell_policy.cc Outdated Show resolved Hide resolved
@nuclearkatie nuclearkatie marked this pull request as draft May 22, 2024 00:14
@nuclearkatie
Copy link
Contributor Author

Ok unfortunately pulling this one back into drafts because as I'm developing it with cycamore#603 something odd is happening. It seems like everything is working well, but then when I try to run a toy test input, like the one here, the simulation aborts... because it can't find any packages.

However, this appears to happen after the package is already found once successfully. I've so far identified that context::GetPackagesByName() is getting called once successfully, and then a second time where it claims to have no packages in its list at all. I'm not yet sure why context::GetPackagesByName() is even being called more than once in a simulation with a single agent using the sell policy and packages (one Storage)??

I'm a bit at a loss here... More digging is definitely needed, it might be the case that I'm missing something in the setup of packages because I'm just not that familiar with the process of spinning up a full simulation

@nuclearkatie
Copy link
Contributor Author

Ok, I think I understand:

  1. The XML file loader loads info from the input file, and makes packages (and recipes and other stuff) through the context, which get recorded in the database
  2. Then a simulation gets initialized, and info including packages are loaded in, this time from the database. However, the way that I previously set up packages, this resulted in packages getting re-created from scratch, which results in new package ids
  3. Now the packages have been read multiple times and confusion ensues

I'm going to fix this by allowing, but not requiring, packages to be created with a package_id. I know we chose not to do this before, but now there is a reason to create a package with a known package id. This will actually bring packages more in line with recipes, which in SimInit actually create a dummy composition with a known QualId (which was read in from the database, where it was created during xml file loading).

But instead of creating dummy packages, I think I'm just going to create the package again? If this is controversial, happy to consider another option, but I don't think we need dummy packages in the same way that dummy compositions are useful.

New workflow

  1. XMLFileLoader loads info from file. Adds unpackaged to database, then creates and adds packages, created with ids
  2. SimInit loads info from database. Creates packages from database info, using the same package id's that were previously created
  3. No extra packages are created, packages can be found in the context as expected for use by matl sell policy (or whatever else)

@nuclearkatie nuclearkatie marked this pull request as ready for review May 23, 2024 18:00
@gonuke
Copy link
Member

gonuke commented May 23, 2024

Ok, I think I understand:

  1. The XML file loader loads info from the input file, and makes packages (and recipes and other stuff) through the context, which get recorded in the database
  2. Then a simulation gets initialized, and info including packages are loaded in, this time from the database. However, the way that I previously set up packages, this resulted in packages getting re-created from scratch, which results in new package ids
  3. Now the packages have been read multiple times and confusion ensues

Why isn't this an issue for recipes? Is it because they don't have IDs? I wonder if we should plan to remove the IDs in the future (outside of PhD scope)?

@gonuke
Copy link
Member

gonuke commented May 23, 2024

Actually... how hard would it be to remove the concept of package_id and just use package names?

@nuclearkatie
Copy link
Contributor Author

Probably not that hard

@nuclearkatie nuclearkatie mentioned this pull request May 29, 2024
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.

This looks good to me. Now that we've changed to package name, I noticed that it's still a little different from how we use recipes: we store the Composition::Ptr for each recipe, rather than the name. I'll make an issue to review this for packages too.

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

Successfully merging this pull request may close these issues.

None yet

3 participants