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

Remove the latticePhysics package and all uses from the framework #1479

Closed
wants to merge 4 commits into from

Conversation

keckler
Copy link
Member

@keckler keckler commented Nov 17, 2023

What is the change?

This removes the latticePhysics package from the framework. This functionality is being brought internal to streamline workflows.

I do not think that anybody outside of TerraPower is using this, but we need to make sure before this gets merged in.

Why is the change being made?

It has never made sense that this feature is in the framework, as it is very tailored to our internal usages. Having this in the framework makes our life very complicated when any part of it needs to be changed, since it requires PRs into multiple repos. Most of what is in the latticePhysics package is a stub anyways, so it really doesn't do anything on its own.


Checklist

  • The release notes (location doc/release/0.X.rst) are up-to-date with any important changes.
  • The documentation is still up-to-date in the doc folder.
  • No requirements were altered.
  • The dependencies are still up-to-date in pyproject.toml.

@keckler
Copy link
Member Author

keckler commented Nov 18, 2023

I am also going to pull the XSGM out of here. So not quite done yet, sorry.

@keckler keckler marked this pull request as draft November 18, 2023 00:05
@john-science john-science self-requested a review November 20, 2023 16:07
@john-science john-science added architecture Issues related to big picture system architecture cleanup Code/comment cleanup: Low Priority labels Nov 20, 2023
@john-science
Copy link
Member

I have two major thoughts on this:

  1. We can't merge this until we check and see what breaks downstream.
  2. Are we sure we want to completely remove these files? I understand ARMI shouldn't include science features like this, but perhaps empty base classes or interface shell should remain?

@john-science
Copy link
Member

@sombrereau @drewj-usnctech Would this change affect you?

The IDEA Chris has here is that this is a science-implementation feature, and thus shouldn't be part of ARMI.

@keckler
Copy link
Member Author

keckler commented Nov 20, 2023

Yes, agreed on both points @john-science .

I will add a couple points just for your consideration:

  1. We are not scrapping this entirely, we are just moving it internal. So Tommy will still have access to it, we just might have to move a couple import statements around.
  2. We got strong buy-in from all the relevant managers in Natrium, so I'm not just doing this as a rogue
  3. This will make our internal SQA effort much much simpler

@albeanth
Copy link
Member

albeanth commented Nov 20, 2023

Yes, agreed on both points @john-science .

I will add a couple points just for your consideration:

  1. We are not scrapping this entirely, we are just moving it internal. So Tommy will still have access to it, we just might have to move a couple import statements around.
  2. We got strong buy-in from all the relevant managers in Natrium, so I'm not just doing this as a rogue
  3. This will make our internal SQA effort much much simpler

This PR should also be against the natrium1 branch, not ARMI main.

The idea of empty base classes, interface shell/skeleton is a good idea for when we migrate this change to ARMI main.

@john-science
Copy link
Member

  1. We are not scrapping this entirely, we are just moving it internal. So Tommy will still have access to it, we just might have to move a couple import statements around.
  2. We got strong buy-in from all the relevant managers in Natrium, so I'm not just doing this as a rogue
  3. This will make our internal SQA effort much much simpler

So, first off (1) is important. We need to know how this affect Tommy/Philip and their team before we break the ARMI API.

For (2) and (3) I believe you misunderstand me. I get what your goal is here. But I am just not convinced by the execution. Convince me! You guys want to just copy/paste the files into a downstream repo. But you are moving a ton of framework-ish stuff out of ARMI too. Sure, the specifics of a lattice physics interface shouldn't be in ARMI. But ARMI shouldn't hold have a base class in it for this?

Let me put it this way: everyone who does nuclear modeling will want to do lattice physics, right? Wouldn't ARMI look strange if we entirely removed even a PLACE for that logic to happen?

Thoughts?

@jakehader
Copy link
Member

Hey @keckler / @john-science - it feels like this could be an impact on @drewj-usnctech or others. Specially we have the Dragon plugin in the public and we should make sure that it still works. I think it would make sense to copy the code internally without removing from the framework and then chip away at abstractions as they make sense. The benefit and downside to the lattice physics interface and XS group manager are that they are pretty opinionated. Great for getting started, but hard to get around if you want the framework to behave differently for your app.

@keckler
Copy link
Member Author

keckler commented Nov 21, 2023

Hey @keckler / @john-science - it feels like this could be an impact on @drewj-usnctech or others. Specially we have the Dragon plugin in the public and we should make sure that it still works. I think it would make sense to copy the code internally without removing from the framework and then chip away at abstractions as they make sense. The benefit and downside to the lattice physics interface and XS group manager are that they are pretty opinionated. Great for getting started, but hard to get around if you want the framework to behave differently for your app.

That's no problem. I can close this PR and just work off of a branch to make sure that everything is working, in the case that we end up wanting to go forward with this removal later on.

@keckler keckler closed this Nov 21, 2023
@keckler
Copy link
Member Author

keckler commented Nov 21, 2023

Please do not delete the associated branch, we are potentially going to pick this up in the next few months.

@keckler
Copy link
Member Author

keckler commented Nov 21, 2023

Note for posterity:

This branch currently has the crossSectionControl (CONF_CROSS_SECTION) removed from the framework. I would like to do this, but it is looking like that might have to be reverted due to it being so frequently used and expected in various places throughout the ecosystem.

@drewj-usnctech
Copy link
Contributor

Late to the party but we are not currently relying on the lattice physics related framework things. We use the xs types in blueprints because we have to use them, but I don't think that's in scope here

@opotowsky opotowsky deleted the removeLatticePhysics branch November 1, 2024 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
architecture Issues related to big picture system architecture cleanup Code/comment cleanup: Low Priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants