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

Package class and declaration of packages in input file #1673

Merged
merged 4 commits into from Feb 21, 2024

Conversation

nuclearkatie
Copy link
Contributor

@nuclearkatie nuclearkatie commented Feb 19, 2024

  • creates Package class
  • packages can be declared in input file (optional)

Closes #1654, closes #1655
Part of #1620

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 pretty good - I think I found the source of your build error.

src/context.h Outdated Show resolved Hide resolved
src/xml_file_loader.cc Show resolved Hide resolved
src/package.h Show resolved Hide resolved
Co-authored-by: Paul Wilson <paul.wilson@wisc.edu>
Signed-off-by: Katie Mummah <radioactivekate@gmail.com>
Copy link

Downstream Build Status Report

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
Copy link
Collaborator

coveralls commented Feb 20, 2024

Pull Request Test Coverage Report for Build 7979730040

Details

  • -20 of 41 (51.22%) changed or added relevant lines in 4 files are covered.
  • 1324 unchanged lines in 45 files lost coverage.
  • Overall coverage decreased (-16.7%) to 32.595%

Changes Missing Coverage Covered Lines Changed/Added Lines %
tests/xml_file_loader_tests.cc 3 4 75.0%
src/context.cc 4 7 57.14%
src/package.cc 6 14 42.86%
src/xml_file_loader.cc 8 16 50.0%
Files with Coverage Reduction New Missed Lines %
src/any.hpp 1 39.72%
src/bid_portfolio.h 1 59.09%
src/env.h 1 53.85%
src/exchange_solver.cc 1 37.68%
src/exchange_translator.h 1 53.03%
src/greedy_preconditioner.cc 1 50.29%
src/logger.cc 1 68.97%
src/material.cc 1 76.74%
src/query_backend.h 1 81.5%
src/recorder.cc 1 74.32%
Totals Coverage Status
Change from base Build 7027753240: -16.7%
Covered Lines: 52970
Relevant Lines: 128938

💛 - 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.

Another question, but I think this is nearly ready to go...

src/package.cc Show resolved Hide resolved
@gonuke
Copy link
Member

gonuke commented Feb 20, 2024

  • How to manage default? Right now I have the packages managed by context, including creating a default regardless if any are declared in the input file. But, packages isn't context-aware, so when I get around to packaging strategies, not sure how best to call the default value

I think the default will always have id=0 and name=default. You could pick a different default name that is more likely to be unique/rare, but I think it should be easy to access the default package through the context.

@nuclearkatie
Copy link
Contributor Author

  • How to manage default? Right now I have the packages managed by context, including creating a default regardless if any are declared in the input file. But, packages isn't context-aware, so when I get around to packaging strategies, not sure how best to call the default value

I think the default will always have id=0 and name=default. You could pick a different default name that is more likely to be unique/rare, but I think it should be easy to access the default package through the context.

So you think package class should have the context as a member? As written, package doesn't have a way to access the context, because it doesn't have a manager or anything

@gonuke
Copy link
Member

gonuke commented Feb 20, 2024

  • How to manage default? Right now I have the packages managed by context, including creating a default regardless if any are declared in the input file. But, packages isn't context-aware, so when I get around to packaging strategies, not sure how best to call the default value

I think the default will always have id=0 and name=default. You could pick a different default name that is more likely to be unique/rare, but I think it should be easy to access the default package through the context.

So you think package class should have the context as a member? As written, package doesn't have a way to access the context, because it doesn't have a manager or anything

No - I'm not sure the package needs to know about the context. I think a resource will access info about available packages via the context and then store a direct link to their current package. Am I missing something about why/how a package would reference its context?

@nuclearkatie
Copy link
Contributor Author

  • How to manage default? Right now I have the packages managed by context, including creating a default regardless if any are declared in the input file. But, packages isn't context-aware, so when I get around to packaging strategies, not sure how best to call the default value

I think the default will always have id=0 and name=default. You could pick a different default name that is more likely to be unique/rare, but I think it should be easy to access the default package through the context.

So you think package class should have the context as a member? As written, package doesn't have a way to access the context, because it doesn't have a manager or anything

No - I'm not sure the package needs to know about the context. I think a resource will access info about available packages via the context and then store a direct link to their current package. Am I missing something about why/how a package would reference its context?

Ah ok, so the default will always be passed through the resource. I was thinking of having a version of package where no package type was passed, and then the package would have to get access to the default

@gonuke
Copy link
Member

gonuke commented Feb 20, 2024

  • How to manage default? Right now I have the packages managed by context, including creating a default regardless if any are declared in the input file. But, packages isn't context-aware, so when I get around to packaging strategies, not sure how best to call the default value

I think the default will always have id=0 and name=default. You could pick a different default name that is more likely to be unique/rare, but I think it should be easy to access the default package through the context.

So you think package class should have the context as a member? As written, package doesn't have a way to access the context, because it doesn't have a manager or anything

No - I'm not sure the package needs to know about the context. I think a resource will access info about available packages via the context and then store a direct link to their current package. Am I missing something about why/how a package would reference its context?

Ah ok, so the default will always be passed through the resource. I was thinking of having a version of package where no package type was passed, and then the package would have to get access to the default

I think that any time we need to know about a package, we'll have access to the context. I'm not sure I can think of a time when a package needs to know about the context without some other code-context to get the info it needs. If we identify something, we'll have to rethink.

@gonuke
Copy link
Member

gonuke commented Feb 20, 2024

If you convert from draft, I can merge this

@nuclearkatie nuclearkatie marked this pull request as ready for review February 20, 2024 23:11
@nuclearkatie
Copy link
Contributor Author

nuclearkatie commented Feb 20, 2024

Looking ahead, resources don't know about the context except when recording, where it is passed to record. So, somewhere is going to need to be context-aware in order to handle the default case. I'll do some more thinking about this...

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 is a great first step on Packaging @nuclearkatie

@gonuke
Copy link
Member

gonuke commented Feb 21, 2024

Sometime soon, coverage will settle down a little and I'll worry about that test, but I'll go ahead now

@gonuke gonuke merged commit 2141f55 into cyclus:main Feb 21, 2024
7 of 8 checks passed
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.

Read in set of Packages and store in Context Design and implement Package class
3 participants