Wangc/convert pp to interface #1508

wangcj05 · 2021-04-09T18:15:11Z

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.

1. Review all computer code.
2. If any changes occur to the input syntax, there must be an accompanying change to the user manual and xsd schema. If the input syntax change deprecates existing input files, a conversion script needs to be added (see Conversion Scripts).
3. Make sure the Python code and commenting standards are respected (camelBack, etc.) - See on the wiki for details.
4. Automated Tests should pass, including run_tests, pylint, manual building and xsd tests. If there are changes to Simulation.py or JobHandler.py the qsub tests must pass.
5. If significant functionality is added, there must be tests added to check this. Tests should cover all possible options. Multiple short tests are preferred over one large test. If new development on the internal JobHandler parallel system is performed, a cluster test must be added setting, in XML block, the node <internalParallel> to True.
6. If the change modifies or adds a requirement or a requirement based test case, the Change Control Board's Chair or designee also needs to approve the change. The requirements and the requirements test shall be in sync.
7. The merge request must reference an issue. If the issue is closed, the issue close checklist shall be done.
8. If an analytic test is changed/added is the the analytic documentation updated/added?
9. If any test used as a basis for documentation examples (currently found in raven/tests/framework/user_guide and raven/docs/workshop) have been changed, the associated documentation must be reviewed and assured the text matches the example.

…tures

PaulTalbot-INL

A couple small comments, but this looks good otherwise. I'm sure this was tedious to convert; thanks for your effort!

framework/BaseClasses/BaseInterface.py

framework/Models/PostProcessors/DataMining.py

framework/Models/PostProcessors/PostProcessorInterface.py

wangcj05 · 2021-04-13T14:54:38Z

@PaulTalbot-INL I have resolved your comments. Thanks.

PaulTalbot-INL

Code changes approved. Continuing with the checklist ...

PaulTalbot-INL · 2021-04-13T14:57:54Z

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

alfoa · 2021-04-20T17:21:57Z

framework/Models/PostProcessor.py

- if Input is not None and len(Input) == 0:
- Input = None
- returnValue = (Input, self.run(Input))
+ ppInput = self.createNewInput(myInput,samplerType, **kwargs)


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

I know it was there arleady, but probably it is a good time to remove it

I agree to remove it.

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.

Please open under discussion issue.

alfoa · 2021-04-20T17:22:39Z

framework/Models/PostProcessor.py

@@ -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)


See above comment. In addition, 'samplerType' is needed, since we are calling the method from Model class.

See comment above. The samplerType should be encpasulated in the kwargs so usable by only the "entities" that need it.

framework/Models/PostProcessor.py

framework/Models/PostProcessors/CrossValidation.py

framework/Models/PostProcessors/PostProcessorInterface.py

alfoa · 2021-04-20T17:30:06Z

framework/Models/PostProcessors/PostProcessorInterface.py

+ 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):


please open an issue. This "error" must be handled at the JobHandler level. We should not reach this point.

alfoa · 2021-04-20T17:30:33Z

framework/Models/PostProcessors/PostProcessorInterface.py

+ dims = outputRealization['dims']
+ else:
+ dims = {}
+ print(outputRealization.keys())


alfoa · 2021-04-20T17:32:14Z

framework/Models/PostProcessors/PostProcessorInterface.py

+ else:
+ dims = {}
+ print(outputRealization.keys())
+ output.load(outputRealization['data'], style='dict', dims=dims)


the load should be modified to also accept a single realizaiton.

PS. I think that the PP must return a dataset. dictionaries are slow

Let's have a discussion for this.

we can, but the "gate" (if statement) should be included in the load method

alfoa · 2021-04-20T17:33:59Z

framework/Models/PostProcessors/__init__.py

+ from .DataMining import QDataMining
+ additionalModules.append(QTopologicalDecomposition)
+ additionalModules.append(QDataMining)
+except ImportError:


please see PR #1503. These double imports are not needed. The only imports needed are in the Factory.py (it was supposed to be addressed in the PR #1480)

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.

I have cleaned up the __init__.py file

wangcj05 · 2021-04-21T17:40:31Z

@alfoa I have resolved your comments.

alfoa · 2021-04-22T19:18:57Z

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.

alfoa · 2021-04-22T19:41:42Z

Checklist passed.

PR approved.

Merging

wangcj05 added 13 commits April 6, 2021 14:22

restructure PostProcessor to use BaseEntities and BaseInterface Struc…

62f663d

…tures

updates

66ffd6f

update

8e5c79a

update super() calls

0bd8aa2

Merge branch 'devel' into wangc/convert_pp_to_interface

7b763d8

update

108918e

enable Assembler object for post processor

ad53cc8

update collectoutput

1339b1d

update

c2186c4

use PP factory instead of Model factory

d6a830f

clean up

cae97e4

fix the way to retrieve the interface properties

cf594ba

add docstr for classes

a91e7a1

wangcj05 requested review from PaulTalbot-INL, alfoa and mandd April 12, 2021 18:54

PaulTalbot-INL requested changes Apr 12, 2021

View reviewed changes

resolve comments

7458d53

PaulTalbot-INL previously approved these changes Apr 13, 2021

View reviewed changes

alfoa requested changes Apr 20, 2021

View reviewed changes

resolve comments

a61bb97

wangcj05 dismissed PaulTalbot-INL’s stale review via a61bb97 April 21, 2021 16:46

alfoa approved these changes Apr 22, 2021

View reviewed changes

alfoa added the RAVENv2.1 All tasks and defects that will go in RAVEN v2.1 label Apr 22, 2021

alfoa mentioned this pull request Apr 22, 2021

[TASK] Restructure PostProcessor based BaseEntity and BaseInterface Structure. #1529

Closed

10 tasks

alfoa merged commit f478787 into devel Apr 22, 2021

alfoa deleted the wangc/convert_pp_to_interface branch April 22, 2021 19:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wangc/convert pp to interface #1508

Wangc/convert pp to interface #1508

wangcj05 commented Apr 9, 2021 •

edited by alfoa

Loading

PaulTalbot-INL left a comment

wangcj05 commented Apr 13, 2021

PaulTalbot-INL left a comment

PaulTalbot-INL commented Apr 13, 2021

alfoa Apr 20, 2021

alfoa Apr 20, 2021

wangcj05 Apr 20, 2021

wangcj05 Apr 21, 2021

alfoa Apr 21, 2021

alfoa Apr 20, 2021

wangcj05 Apr 21, 2021

alfoa Apr 21, 2021 •

edited

Loading

alfoa Apr 20, 2021

wangcj05 Apr 21, 2021

alfoa Apr 20, 2021

wangcj05 Apr 21, 2021

alfoa Apr 20, 2021

wangcj05 Apr 20, 2021

alfoa Apr 21, 2021

alfoa Apr 20, 2021

PaulTalbot-INL Apr 20, 2021

wangcj05 Apr 21, 2021 •

edited

Loading

wangcj05 commented Apr 21, 2021

alfoa commented Apr 22, 2021

alfoa commented Apr 22, 2021

Wangc/convert pp to interface #1508

Wangc/convert pp to interface #1508

Conversation

wangcj05 commented Apr 9, 2021 • edited by alfoa Loading

Pull Request Description

What issue does this change request address? (Use "#" before the issue to link it, i.e., #42.)

What are the significant changes in functionality due to this change request?

For Change Control Board: Change Request Review

PaulTalbot-INL left a comment

Choose a reason for hiding this comment

wangcj05 commented Apr 13, 2021

PaulTalbot-INL left a comment

Choose a reason for hiding this comment

PaulTalbot-INL commented Apr 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alfoa Apr 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangcj05 Apr 21, 2021 • edited Loading

Choose a reason for hiding this comment

wangcj05 commented Apr 21, 2021

alfoa commented Apr 22, 2021

alfoa commented Apr 22, 2021

wangcj05 commented Apr 9, 2021 •

edited by alfoa

Loading

alfoa Apr 21, 2021 •

edited

Loading

wangcj05 Apr 21, 2021 •

edited

Loading