8000 Allow XML to have multiple `archetype` elements by gonuke · Pull Request #1874 · cyclus/cyclus · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Allow XML to have multiple archetype elements #1874

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

Merged
merged 4 commits into from
Jul 3, 2025

Conversation

gonuke
Copy link
Member
@gonuke gonuke commented Jul 2, 2025

The previous XML schema only allowed a single archetype block to exist. This change allows multiple archetype blocks, as long as there is at least one.

A motivating use case for this is to facilitate including XML snippets that contain long lists of facility prototypes that may be useful in many simulations. Without this change, the top level input file must include all the archetype specs for the archetypes that are used in those facilities. With this change, the archetype specs that are used in that included file can be included there to make sure they are available.

Fortunately, this required no code changes and only a change in our XML schema. We already processed archetypes in a way that didn't care how many blocks there were, nor cared if there were duplicates of the same archetype.

A test has been added that demonstrates how to use this kind of inclusion, which requires the xpointer metadata in the include line.

@gonuke gonuke marked this pull request as ready for review July 2, 2025 21:19
@gonuke gonuke requested review from abachma2 and dean-krueger July 2, 2025 21:19
@coveralls
Copy link
Collaborator
coveralls commented Jul 2, 2025

Pull Request Test Coverage Report for Build 16057951560

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 41.232%

Totals Coverage Status
Change from base Build 16034058723: 0.0%
Covered Lines: 51927
Relevant Lines: 125939

💛 - Coveralls

Copy link
github-actions bot commented Jul 2, 2025

Downstream Build Status Report - 43e32f8 - 2025-07-03 18:29:02 +0000

Build FROM cyclus_20.04_apt/cyclus
  • Cycamore: Success
  • Cymetric: Failure
Build FROM cyclus_20.04_apt/cyclus --parallel
  • Cycamore: Success
  • Cymetric: Failure
Build FROM cyclus_20.04_conda/cyclus
  • Cycamore: Success
  • Cymetric: Failure
Build FROM cyclus_20.04_conda/cyclus --parallel
  • Cycamore: Success
  • Cymetric: Failure
Build FROM cyclus_22.04_apt/cyclus
  • Cycamore: Success
  • Cymetric: Failure
Build FROM cyclus_22.04_apt/cyclus --parallel
  • Cycamore: Success
  • Cymetric: Failure
Build FROM cyclus_22.04_conda/cyclus
  • Cycamore: Success
  • Cymetric: Failure
Build FROM cyclus_22.04_conda/cyclus --parallel
  • Cycamore: Success
  • Cymetric: Failure
Build FROM cyclus_24.04_apt/cyclus
  • Cycamore: Success
  • Cymetric: Failure
Build FROM cyclus_24.04_apt/cyclus --parallel
  • Cycamore: Success
  • Cymetric: Failure
Build FROM cyclus_24.04_conda/cyclus
  • Cycamore: Success
  • Cymetric: Failure
Build FROM cyclus_24.04_conda/cyclus --parallel
  • Cycamore: Success
  • Cymetric: Failure

Copy link
Member
@abachma2 abachma2 left a comment

Choose a reason for hiding this comment

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

Thanks for making this addition @gonuke. I can see how this can add flexibility for a user to make more modular files. I have a few comments and suggestions.

clean_outs()
# Cyclus simulation input for recipe including
sim_input = os.path.join(INPUT, "include_source_w_archetype.xml")
holdsrtn = [1] # needed because nose does not send() to test generator
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 still need this if we don't use nose? Maybe this is outside the scope of this PR and should be a new issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

great question! I'm not sure where else things like this may exist, so it might be worth a new issue to clean these up

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good. I'll make a new issue for that then.

Improved documentation and comments

Co-authored-by: Amanda Bachmann <abachmann@anl.gov>
Signed-off-by: Paul Wilson <paul.wilson@wisc.edu>
Copy link
Member
@abachma2 abachma2 left a comment

Choose a reason for hiding this comment

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

Thanks for making these changes @gonuke. I'm approving and will merge once I see the Cyclus tests passing.

Copy link
Contributor
@dean-krueger dean-krueger left a comment

Choose a reason for hiding this comment

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

Had to go fast to sneak in before Amanda could merge, but this looks good to me as well. Excited to try this out sometime!

@gonuke
Copy link
Member Author
gonuke commented Jul 3, 2025

Had to go fast to sneak in before Amanda could merge, but this looks good to me as well. Excited to try this out sometime!

I think @trysarahtops will be eager to try this out to make her input files simpler!

@abachma2
Copy link
Member
abachma2 commented Jul 3, 2025

All the tests passed. Merging now.

@abachma2 abachma2 merged commit 76fb89f into cyclus:main Jul 3, 2025
26 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.

4 participants
0