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 unused parameters related to reactivity coefficients #1501

Merged
merged 6 commits into from
Dec 1, 2023

Conversation

keckler
Copy link
Member

@keckler keckler commented Nov 28, 2023

What is the change?

This PR is a big housecleaning of a bunch of parameters that were related to our old internal implementation of reactivity coefficient calculations. They are not used anywhere. I have run our internal integration tests and found no issues.

Why is the change being made?

These parameters are unused bloat. They should have been removed a long time ago. Many of them are ambiguous. Having these around makes working with parameters programmatically difficult.

Side note: These parameters would more properly belong in a plugin, since they are very specific and not used by multiple plugins.


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 keckler requested a review from john-science November 28, 2023 01:30
@keckler
Copy link
Member Author

keckler commented Nov 28, 2023

@john-science We should talk about this in person tomorrow, if you have some time.

@john-science john-science added the cleanup Code/comment cleanup: Low Priority label Nov 28, 2023
@john-science
Copy link
Member

Chris and I have reviewed this.

This PR is tacitly approved, based on some testing Chris is running.

But also, I'm not going to hit "Approve" here until after the testing is complete (~24 hours), because I don't want someone YOLO-merging this PR just yet.

@keckler
Copy link
Member Author

keckler commented Nov 29, 2023

@john-science My internal testing came back clean. This consisted of running our integration tests and also a benchmark case that exercises this part of the code.

@john-science
Copy link
Member

Test coverage only went down because we removed lines of code.

That is exceptable.

@keckler
Copy link
Member Author

keckler commented Nov 30, 2023

Thanks @john-science . I can coordinate the merge, since there are a few internal repos that will be merged simultaneously.

@keckler
Copy link
Member Author

keckler commented Nov 30, 2023

It was pointed out to me by @mgjarrett that I shouldn't remove rxFuelAxialExpansionPerPercent because it is planned to be used very soon by an internal plugin. Does it belong in that plugin instead of the framework? 😏

@keckler keckler merged commit 77623d5 into main Dec 1, 2023
21 checks passed
@keckler keckler deleted the removeUnusedRxcoeffParams branch December 1, 2023 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Code/comment cleanup: Low Priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants