-
Notifications
You must be signed in to change notification settings - Fork 132
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
Wangc/convert pp to interface #1508
Conversation
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.
A couple small comments, but this looks good otherwise. I'm sure this was tedious to convert; thanks for your effort!
@PaulTalbot-INL I have resolved your comments. Thanks. |
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.
Code changes approved. Continuing with the checklist ...
With the HPC down this week, we're stuck waiting on the tests unless @alfoa green lights an alternate testing strategy. I only have access to OSX myself right now. |
framework/Models/PostProcessor.py
Outdated
if Input is not None and len(Input) == 0: | ||
Input = None | ||
returnValue = (Input, self.run(Input)) | ||
ppInput = self.createNewInput(myInput,samplerType, **kwargs) |
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.
I am conflicted here again. the samplerType is set at the Step stage...so for PostProcessor steps it will always be None. I dnk but I think it is not needed and it can cause confusion for the PostProcessor developer
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.
I know it was there arleady, but probably it is a good time to remove it
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.
I agree to remove it.
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.
I have updated the docstring for better clarification. I think we can keep "samplerType" which will consistent with other Models at entity level. In addition, these methods will only at entity level, and we will not expose this method to post-processor developer. In other words, the pp developers will not see these methods, so there is no confusion for them.
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.
Please open under discussion issue.
@@ -207,9 +210,9 @@ def submit(self,myInput,samplerType,jobHandler,**kwargs): | |||
@ Out, None | |||
""" | |||
kwargs['forceThreads'] = True | |||
Model.submit(self,myInput, samplerType, jobHandler,**kwargs) | |||
super().submit(myInput, samplerType, jobHandler,**kwargs) |
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.
as above
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.
See above comment. In addition, 'samplerType' is needed, since we are calling the method from Model 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.
See comment above. The samplerType should be encpasulated in the kwargs so usable by only the "entities" that need it.
if output.type not in self.validDataType: | ||
self.raiseAnError(IOError, 'Output type', str(output.type), 'is not allowed!') | ||
evaluation = finishedJob.getEvaluation() | ||
if isinstance(evaluation, Runners.Error): |
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.
please open an issue. This "error" must be handled at the JobHandler level. We should not reach this point.
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.
see #1524
dims = outputRealization['dims'] | ||
else: | ||
dims = {} | ||
print(outputRealization.keys()) |
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.
remove
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.
removed.
else: | ||
dims = {} | ||
print(outputRealization.keys()) | ||
output.load(outputRealization['data'], style='dict', dims=dims) |
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.
the load should be modified to also accept a single realizaiton.
PS. I think that the PP must return a dataset. dictionaries are slow
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.
Let's have a discussion for this.
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.
we can, but the "gate" (if statement) should be included in the load method
from .DataMining import QDataMining | ||
additionalModules.append(QTopologicalDecomposition) | ||
additionalModules.append(QDataMining) | ||
except ImportError: |
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.
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.
I think that observation came after that PR, right? Either way, I concur, I like the idea that classes are only available through the factories. This may require some standardization so we can check types consistently; previously we would import the module and check instance, such as
from Samplers import MonteCarlo
if isinstance(mySampler, MonteCarlo):
...
This might be harder with factories, but should be standardizable without checking against strings.
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.
I have cleaned up the __init__.py
file
@alfoa I have resolved your comments. |
Some regression test machines have connectivity issues currently under investigation by @joshua-cogliati-inl . The tests have been run locally and are green. The PR is approved and can be merged. |
Checklist passed. PR approved. Merging |
Pull Request Description
What issue does this change request address? (Use "#" before the issue to link it, i.e., #42.)
Restructure PostProcessor Entity and PostProcessor Interface. Let PostProcessor Entity inherit from Model, and PostProcessor Interface inherit form BaseInterface.
See #1486
Close #1529
What are the significant changes in functionality due to this change request?
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.