Package management by context and handling of repackaging process #1729

nuclearkatie · 2024-04-11T16:35:54Z

Fix unpackaged (default) package type. Closes Unpackaged id gets created as id 2, id 1 is not actually tied to a package #1730
Ensure that creating new packages is done through the context (context::AddPackage), which calls Package::Create. Closes New package types can't just be created, they must also be given to the context to manage #1731
Package-filling process managed by resource, not by package. Closes Resolve templating for Pacakge functions #1714

github-actions · 2024-04-11T16:50:30Z

Downstream Build Status Report - `7092890` - 2024-04-30 14:57:21 -0600

Build `FROM cyclus_20.04_apt/cyclus`

Cycamore: Success ✅
Cymetric: Success ✅

Build `FROM cyclus_20.04_conda/cyclus`

Cycamore: Success ✅
Cymetric: Success ✅

Build `FROM cyclus_22.04_apt/cyclus`

Cycamore: Success ✅
Cymetric: Success ✅

Build `FROM cyclus_22.04_conda/cyclus`

Cycamore: Success ✅
Cymetric: Success ✅

coveralls · 2024-04-11T16:51:53Z

Pull Request Test Coverage Report for Build 8901490530

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

77 of 84 (91.67%) changed or added relevant lines in 6 files are covered.
5 unchanged lines in 3 files lost coverage.
Overall coverage increased (+8.2%) to 40.771%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/context.cc	3	4	75.0%
src/resource.h	9	10	90.0%
src/package.cc	17	22	77.27%

Files with Coverage Reduction	New Missed Lines	%
tests/toolkit/infile_converters_tests.cc	1	96.88%
src/package.cc	2	74.07%
build/cyclus/py_lib_.cxx	2	45.0%

Totals
Change from base Build 8680397273:	8.2%
Covered Lines:	53225
Relevant Lines:	130546

💛 - Coveralls

gonuke

With apologies to another flip-flop (let's discuss) here are some otherwise minor comments.

src/resource.cc

gonuke

I think the initialization of the unpackaged package in the context has gotten more complicated than it needs to... unless I'm forgetting something about how it all works.

src/context.cc

src/package.cc

gonuke · 2024-04-15T22:05:53Z

src/material.cc

+
+  while (quantity() > pkg->fill_min()) {
+    double pkg_fill = std::min(quantity(), fill_mass);
+    m_pkgd = boost::dynamic_pointer_cast<Material>(ExtractRes(pkg_fill));


seems like this is the only line that really forces this method to be replicated 3 times... I'll have to dust off my C++ polymorphism to see if there's a better way...

src/package.h

src/product.cc

try singleton pattern for unpackaged

gonuke

One error here

src/package.h

gonuke

Now that we've resolved the main design questions, a few final cleanup changes.

src/package.h

src/context.h

src/resource.cc

src/resource.h

src/product.h

src/material.h

Co-authored-by: Paul Wilson <paul.wilson@wisc.edu> Signed-off-by: Katie Mummah <radioactivekate@gmail.com>

gonuke

These comments have the fix for the current issue, but not sure what it will mean for nuances when we call these methods.

src/material.h

src/product.h

Co-authored-by: Paul Wilson <paul.wilson@wisc.edu> Signed-off-by: Katie Mummah <radioactivekate@gmail.com>

promote Package to parent class with templating

gonuke

Suggest a couple of additional tests.

gonuke · 2024-04-26T18:31:28Z

tests/resource_tests.cc

+  // all packaged
+  std::vector<Product::Ptr> p1_pkgd = p1->Package<Product>(pkg);
+  EXPECT_EQ(p1->quantity(), 0);
+  EXPECT_EQ(p1_pkgd[0]->package_id(), pkg_id);


should we also check the number of packages? If I read this right, p1_pkgd should be length one: all of its 3 units of quantity in a single package.

We could also package p2 and its 7 units would end up in two packages of size 5 and 2

you'll never guess what was sitting forgotten in my stashes.... though I didn't I have the two packages case

src/context.h

nuclearkatie added the Defect label Apr 11, 2024

nuclearkatie added this to the Bulk-to-Discrete packaging milestone Apr 11, 2024

gonuke requested changes Apr 11, 2024

View reviewed changes

src/resource.cc Outdated Show resolved Hide resolved

src/resource.cc Outdated Show resolved Hide resolved

src/resource.cc Outdated Show resolved Hide resolved

nuclearkatie changed the title ~~move GetFillMass and PackageResource to resource class~~ Package management by context and handling of repackaging process Apr 11, 2024

nuclearkatie added 7 commits April 15, 2024 14:23

move GetFillMass and PackageResource to resource class

9dc611e

split GetFillMass and PackageResource

d70e503

upon intialization, context creates unpackaged type

efa4ed9

allow creation of unpackaged type with id always =1

215f588

repackage

1499ce7

resource packaging tests

0d232f2

changelog

1d14fd3

nuclearkatie force-pushed the packaging branch from 5474219 to 1d14fd3 Compare April 15, 2024 20:27

don't have to dynamic cast resource after extract

4fad47d

nuclearkatie marked this pull request as ready for review April 15, 2024 20:49

gonuke requested changes Apr 15, 2024

View reviewed changes

nuclearkatie and others added 11 commits April 15, 2024 16:26

remove unused old package call

219e113

unpackaged name

cfd94e3

no default create package

0983c42

try singleton pattern for unpackaged

ab3c697

add const

f530690

pass the unpackaged by reference

9b44b17

fix reference location

8f3e31a

fix initializers

ec39b33

remove consts

359e96f

resolve build final errors

3b78c8d

tidy up construction in singleton accesor

9b95834

nuclearkatie mentioned this pull request Apr 16, 2024

try singleton pattern for unpackaged nuclearkatie/cyclus#2

Merged

Merge pull request #2 from gonuke/unpackage_singleton

df2fdf4

try singleton pattern for unpackaged

nuclearkatie added 2 commits April 16, 2024 12:30

defaults for package create

b508d8d

default strategy

e8fd577

gonuke reviewed Apr 17, 2024

View reviewed changes

src/package.h Outdated Show resolved Hide resolved

gonuke requested changes Apr 17, 2024

View reviewed changes

nuclearkatie and others added 4 commits April 17, 2024 10:09

Make package virtual - Apply suggestions from code review

fa988fb

Co-authored-by: Paul Wilson <paul.wilson@wisc.edu> Signed-off-by: Katie Mummah <radioactivekate@gmail.com>

add package returns void

8f47fcb

no friend class needed for package

6e63f52

no package in resource

391b5d7

gonuke requested changes Apr 20, 2024

View reviewed changes

src/material.h Outdated Show resolved Hide resolved

src/product.h Outdated Show resolved Hide resolved

nuclearkatie and others added 4 commits April 22, 2024 08:45

Return resource type - Apply suggestions from code review

7264510

Co-authored-by: Paul Wilson <paul.wilson@wisc.edu> Signed-off-by: Katie Mummah <radioactivekate@gmail.com>

cast to resource?

dc6d366

promote Package to parent class with templating

a402c01

Merge pull request #3 from gonuke/mv_packaging

29fb87d

promote Package to parent class with templating

gonuke requested changes Apr 26, 2024

View reviewed changes

nuclearkatie added 2 commits April 30, 2024 14:50

test packaged size

8684292

indenting

7092890

gonuke approved these changes Apr 30, 2024

View reviewed changes

gonuke merged commit ff37df4 into cyclus:main Apr 30, 2024
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Package management by context and handling of repackaging process #1729

Package management by context and handling of repackaging process #1729

nuclearkatie commented Apr 11, 2024 •

edited

github-actions bot commented Apr 11, 2024 •

edited

coveralls commented Apr 11, 2024 •

edited

gonuke left a comment

gonuke left a comment

gonuke Apr 15, 2024

gonuke left a comment

gonuke left a comment

gonuke left a comment

gonuke left a comment

gonuke Apr 26, 2024

nuclearkatie Apr 30, 2024 •

edited

Package management by context and handling of repackaging process #1729

Package management by context and handling of repackaging process #1729

Conversation

nuclearkatie commented Apr 11, 2024 • edited

github-actions bot commented Apr 11, 2024 • edited

Downstream Build Status Report - 7092890 - 2024-04-30 14:57:21 -0600

Build FROM cyclus_20.04_apt/cyclus

Build FROM cyclus_20.04_conda/cyclus

Build FROM cyclus_22.04_apt/cyclus

Build FROM cyclus_22.04_conda/cyclus

coveralls commented Apr 11, 2024 • edited

Pull Request Test Coverage Report for Build 8901490530

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

gonuke left a comment

Choose a reason for hiding this comment

gonuke left a comment

Choose a reason for hiding this comment

gonuke Apr 15, 2024

Choose a reason for hiding this comment

gonuke left a comment

Choose a reason for hiding this comment

gonuke left a comment

Choose a reason for hiding this comment

gonuke left a comment

Choose a reason for hiding this comment

gonuke left a comment

Choose a reason for hiding this comment

gonuke Apr 26, 2024

Choose a reason for hiding this comment

nuclearkatie Apr 30, 2024 • edited

Choose a reason for hiding this comment

nuclearkatie commented Apr 11, 2024 •

edited

github-actions bot commented Apr 11, 2024 •

edited

Downstream Build Status Report - `7092890` - 2024-04-30 14:57:21 -0600

Build `FROM cyclus_20.04_apt/cyclus`

Build `FROM cyclus_20.04_conda/cyclus`

Build `FROM cyclus_22.04_apt/cyclus`

Build `FROM cyclus_22.04_conda/cyclus`

coveralls commented Apr 11, 2024 •

edited

nuclearkatie Apr 30, 2024 •

edited