Make additional use of ParameterGroup#404
Open
mabruzzo wants to merge 10 commits intoenzo-project:mainfrom
Open
Make additional use of ParameterGroup#404mabruzzo wants to merge 10 commits intoenzo-project:mainfrom
ParameterGroup#404mabruzzo wants to merge 10 commits intoenzo-project:mainfrom
Conversation
…meterGroup (the parameters are still curently stored in Config, but this should help us remove the relative parameter-path stuff from Parameters down the line.
…oMethodCheck (this has not been moved out of EnzoConfig quite yet.
…f_parameter_names() This argumentless version depended on the group that is currently tracked by the Parameters class.
Contributor
|
@mabruzzo it seems that a bunch of tests are failing here. It looks like it'll be easy to fix - could you do that when you get a second, and we'll then review this PR? |
bwoshea
approved these changes
Jan 16, 2025
Contributor
bwoshea
left a comment
bwoshea
left a comment
There was a problem hiding this comment.
The changes are straightforward and I don't have any specific comments. I will approve this PR, but I note that there is something wrong with CircleCI and I can't even SEE the tests, much less get them to run. @mabruzzo if you can force the tests to run and they pass I think it's fine to merge this!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull request summary
Modify some existing parsing logic to make use of
ParameterGroup.Detailed Description
The
ParameterGroupclass was introduced back in #341 (the main body of that PR calls the classParameterAccessor-- the name was changed based on feedback). It is a class whose sole-purpose is to provide access to parameters within a specific parameter-group (without specifying the full path to a parameter).This functionality was duplicated in the pre-existing
Parametersclass. However, theParametersclass already provided a LOT of other functionality and this particular functionality is achieved by managing a bunch of internal state.Part of the intention when introducing
ParameterGroupwas to eventually replace existing code that already made use of this functionality provided by theParametersclass so that it now makes use of theParametersGroupclass instead (in other words, the underlying logic is not changing). This is what this PR does.The benefits are twofold:
Parametersclass (this will actually be somewhat helpful for simplifying code in SMP-mode).I plan to remove the duplicated functionality in a future, separate PR since it will involve a number of modifications to existing unit-tests & it would be helpful to get the current changes into the main codebase. (Plus these changes have been kicking around on my computer for 3 months and I want to get them off my plate)
EDIT: MAKE SURE THAT YOU SEE THAT ALL CIRCLECI TESTS HAVE PASSED BEFORE MERGING, THEY DON'T ALL SEEM TO APPEAR RIGHT NOW