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

Allow merging a solute with zero area #1858

Merged
merged 6 commits into from
Sep 10, 2024
Merged

Conversation

mgjarrett
Copy link
Contributor

What is the change?

The block converter mergeComponents was implemented to not allow merging any components that had zero area. This should be loosened to allow for merging a zero area solute into a non-zero area solvent.

Why is the change being made?

There are many situations where we don't care about modeling zero-area components (for example, a gap that has completely closed) and it is helpful to be able to merge these components into another component so that they don't appear on the block.


Checklist

  • The release notes have been updated if necessary.
  • The documentation is still up-to-date in the doc folder.
  • The dependencies are still up-to-date in pyproject.toml.

@john-science john-science self-requested a review September 6, 2024 12:23
@john-science john-science added the feature request Smaller user request label Sep 6, 2024
from armi import runLog
from armi.reactor import blocks, components, grids
Copy link
Member

Choose a reason for hiding this comment

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

I see you keep doing this. And, fine.

Just to be clear, I don't actually think this improve readability. And I have explicitly done the opposite in hundreds of ARMI files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's being done automatically by my linter, maybe I don't have it configured correctly?

Copy link
Member

Choose a reason for hiding this comment

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

huh

Copy link
Member

@john-science john-science left a comment

Choose a reason for hiding this comment

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

It looks good to me.

Have you tested this downstream yet?

@mgjarrett
Copy link
Contributor Author

It looks good to me.

Have you tested this downstream yet?

Not yet, no. I don't expect any issues but I still need to test it.

@john-science john-science merged commit 2f95074 into main Sep 10, 2024
21 checks passed
@john-science john-science deleted the merge_zero_area_solute branch September 10, 2024 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Smaller user request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants