-
Notifications
You must be signed in to change notification settings - Fork 133
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
[TASK] Allow functions in samplers/optimizers to be implemented in the same python module #2301
Conversation
Job Mingw Test on f85cedd : invalidated by @alfoa |
Job Mingw Test on f85cedd : invalidated by @aalfonsi |
@wangcj05 @mandd @joshua-cogliati-inl the windows machine does not seem to work |
Job Mingw Test on f85cedd : invalidated by @wangcj05 |
Job Mingw Test on f85cedd : invalidated by @joshua-cogliati-inl failed in fetch |
Job Mingw Test on f85cedd : invalidated by @alfoa fetch error |
@alfoa FYI, our HPC is under maintenance right now. This outage will last until May 6. Please let me know if you need to merge this PR before that. |
@wangcj05 No problem for me. I can use my local branch for now. Let me know if you have comments on this in the meanwhile (that I can address offline). Thanks a lot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alfoa I have some comments for you to consider.
fPointer = namedtuple("func", ['methodName', 'instance']) | ||
mName = 'evaluate' | ||
# check if the correct method is present | ||
if "evaluate" not in self.funcDict[key].availableMethods(): | ||
self.raiseAnError(IOError, f'Function {self.funcDict[key].name} does not contain a method named "evaluate". It must be present if this needs to be used in a Sampler!') | ||
if val not in initDict['Functions'][val].availableMethods(): | ||
if "evaluate" not in initDict['Functions'][val].availableMethods(): | ||
self.raiseAnError(IOError, f'Function {initDict["Functions"][val].name} does contain neither a method named "{val}" nor "evaluate". ' | ||
'It must be present if this needs to be used in a Sampler!') | ||
else: | ||
mName = val | ||
self.funcDict[key] = fPointer(mName, initDict['Functions'][val]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can these lines be handled in the Sampler base class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately it seems that the CustomSampler and the EnsembleForward are "special" cases and they take care of they functions directly. I did not add this "exception" with this implementation but it was like that already unfortunately.
self.funcDict[key] = availableFunc[val] | ||
fPointer = namedtuple("func", ['methodName', 'instance']) | ||
mName = 'evaluate' | ||
# check if the correct method is present | ||
if "evaluate" not in self.funcDict[key].availableMethods(): | ||
self.raiseAnError(IOError, f'Function {self.funcDict[key].name} does not contain a method named "evaluate". It must be present if this needs to be used in a Sampler!') | ||
if val not in availableFunc[val].availableMethods(): | ||
if "evaluate" not in availableFunc[val].availableMethods(): | ||
self.raiseAnError(IOError, f'Function {availableFunc[val].name} does contain neither a method named "{val}" nor "evaluate". ' | ||
'It must be present if this needs to be used in a Sampler!') | ||
else: | ||
mName = val | ||
self.funcDict[key] = fPointer(mName, availableFunc[val]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can these lines be handled in the Sampler base class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately it seems that the CustomSampler and the EnsembleForward are "special" cases and they take care of they functions directly. I did not add this "exception" with this implementation but it was like that already unfortunately.
Job Mingw Test on f85cedd : invalidated by @joshua-cogliati-inl failed in fetch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changes are good.
Checklist is good, PR can be merged. |
@alfoa Thanks for your contribution, I have merged your PR. |
Pull Request Description
What issue does this change request address? (Use "#" before the issue to link it, i.e., #42.)
Closes #2300
What are the significant changes in functionality due to this change request?
This PR Allows functions in samplers/optimizers to be implemented in the same python module.
The old approach (method named ``evaluate'' is still available) but a new approach where a method named like the function is implemented.
@wangcj05 @mandd @idaholab/raven-team This is a modification I did in my local version and I thought it could be useful. Feel free to close/discard this feature/PR.
For Change Control Board: Change Request Review
The following review must be completed by an authorized member of the Change Control Board.
<internalParallel>
to True.raven/tests/framework/user_guide
andraven/docs/workshop
) have been changed, the associated documentation must be reviewed and assured the text matches the example.