Postprocessor plugin APIs #1512

wangcj05 · 2021-04-13T19:29:48Z

Pull Request Description

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

see #1486
Closes #1451

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

Standardize the APIs for Postprocessor Plugin.

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

wangcj05 · 2021-04-13T19:40:13Z

@alfoa @mandd @PaulTalbot-INL I have changed the PostProcessor Plugin base class inheritance. Please take a look, any comments are appreciated.

PaulTalbot-INL · 2021-04-13T20:10:49Z

I think this is a good approach.

Note we don't have an automatic system for loading the plugins from outside RAVEN into the factories in RAVEN; I started on it but have not been able to finish that yet.

PaulTalbot-INL · 2021-04-19T16:18:24Z

framework/PluginsBaseClasses/PostProcessorPluginBase.py

+ @ In, paramInput, InputData.ParameterInput, the already parsed input.
+ @ Out, None
+ """
+ super()._handleInput(paramInput)


My only thought from the plugin side is the somewhat confusing usage of the term "input".

In handleInput it means "handle input XML from the RAVEN input file" whereas in createNewInput it means "prepare the input for a single postprocessor run". I wonder how (or even if) we could approach that to make it more clear for plugin developers.

framework/Models/PostProcessors/DataClassifier.py

alfoa · 2021-04-20T16:32:04Z

framework/Models/PostProcessor.py

@@ -175,6 +175,8 @@ def createNewInput(self,myInput,samplerType,**kwargs):
 a mandatory key is the sampledVars'that contains a dictionary {'name variable':value}
 @ Out, myInput, list, the inputs (list) to start from to generate the new one
 """
+ if 'createNewInput' in dir(self._pp):
+ myInput = self._pp.createNewInput(myInput,samplerType,**kwargs)


should we remove the "samplerType" (I know it was present already)? Postprocesessors do not use sampleType or we are thinking here to extend the possibility to have PP behave differently depending on the Sampler that generated the data? (e.g. Sortino -> Sobol, OtherSamplers -> least square of HDMR coefficients, etc,)?

Considering also Paul's comment, I suggest to change the function name "createNewInput(myInput,samplerType,**kwargs)" to something like "processInput(myInput, **kwargs)". The "samplerType" will be removed.

I wonder if we could clarify which input, like "createPostprocessorInput" or "createModelInput"; I feel like the term "input" is the confusing one; or maybe even "prepareModelInput"?

changed to use "createPostprocessorInput" and "samplerType" is removed.

alfoa · 2021-04-20T16:32:56Z

framework/Models/PostProcessors/DataClassifier.py

- self.raiseAnError(IOError, "Only PointSet is allowed as classifier, but HistorySet", inputObject.name, "is provided!")
+ requiredKeys = list(self.mapping.keys()) + [self.label]
+ for inputDict in currentInput:
+ print(inputDict['type'])


alfoa · 2021-04-20T16:34:35Z

framework/Models/PostProcessors/DataClassifier.py

- understandable by this pp.
- @ In, currentInput, list, a list of DataObjects
- @ Out, newInput, list, list of converted data
+ Method to identify the inputs for classifier and target, respectively


can you expand the description here? What "identify" means?

alfoa · 2021-04-20T16:35:13Z

framework/Models/PostProcessors/DataClassifier.py

- @ In, currentInput, list, a list of DataObjects
- @ Out, newInput, list, list of converted data
+ Method to identify the inputs for classifier and target, respectively
+ @ In, currentInput, list, a list of dictionaries


list of dictionaries...what is in these? This defines the API (e.g. {'data':array, 'dims':list(dimensions), etc}

I agree with this comment, there are few things as confusing as trying to interrogate a dictionary for what should be there. I wonder if we could explicitly list the contents as args and kwargs instead of as a summary input object for the interface objects.

alfoa · 2021-04-20T16:35:47Z

framework/Models/PostProcessors/DataClassifier.py

 return newInput

 def run(self, inputIn):
 """
 This method executes the postprocessor action.
- @ In, inputIn, list, list of DataObjects
+ @ In, inputIn, list, list of input dictionaries


please define what those dictionaries look like (what do we have inside?)

e.g. dims, type

alfoa · 2021-04-20T16:38:49Z

framework/Models/PostProcessors/Factory.py

@@ -39,6 +39,7 @@
 from .ParetoFrontierPostProcessor import ParetoFrontier
 from .MCSimporter import MCSImporter
 from .EconomicRatio import EconomicRatio
+from .RiskMeasuresDiscrete import RiskMeasuresDiscrete


I am confused about this "double" import (both in the __init__ and Factory.py).

See my PR about the validation gate, the double import is not needed as long as the imports are in the Factory.py. This is valid for all the Factories that have been "Restructured" with PR #1480

That's good to know, we can do a quick clean up in another PR, since I do not see the double imports get removed.

yes but we can remove it from here

the __init__ got cleaned up in previous PR.

alfoa · 2021-04-20T16:41:23Z

framework/PluginsBaseClasses/PostProcessorPluginBase.py

+ # Set default to 'dict', this is consistent with current post-processors
+ outType = kwargs.get('outType', 'dict')
+ inputDs.append(inp.asDataset(outType=outType))
+ elif isinstance(inp, Database):


all these checks should be moved at the validation stage in the Models.py (I thought we already have it. Doesn't it work?). Databases are not possible for PP and they should be checked for all of them at once

alfoa · 2021-04-20T16:44:08Z

framework/DataObjects/DataSet.py

+ dataDict['outVars'] = self.getVars('output')
+ dataDict['numberRealization'] = self.size
+ dataDict['name'] = self.name
+ dataDict['metaKeys'] = self.getVars('meta')


I am conflicted here (I mean for the usage of this for PP). I do not think we should use dictionaries anymore in PP). The data should be provided as Dataset only. What do you think?

I wonder if we can provide sufficiently clear examples for developers so that Datasets are easy for them to work with for the things they need to do; otherwise, I worry this could be a big barrier to external development, since the xarray datasets can be difficult until you've played with them a bit, at least for me.

I leave the option to use either Dict or Dataset. Currently, we only have BasicStatistics to use Dataset. With this option, we can identify the PP that we need to convert to use Dataset, and convert them one by one. Otherwise, we can not merge the PP changes without converting all PPs to use Dataset. In addition, I'm not sure it is worth to convert our PPs to use Dataset if there is no need from our customer.

joshua-cogliati-inl · 2021-04-21T14:24:46Z

tests/framework/PostProcessors/RiskMeasuresDiscrete/test_riskMeasuresDiscrete.xml

 <Models>
 <ExternalModel name='PythonModuleReduced' subType='' ModuleToLoad='modelTHreduced'>
 <variables>pump1Time,pump2Time,valveTime,Tmax,outcome,pump1State,pump2State,valveState,failureTime</variables>
 </ExternalModel>
- <PostProcessor name="riskMeasuresDiscrete" subType="InterfacedPostProcessor">
+ <PostProcessor name="riskMeasuresDiscrete" subType="RiskMeasuresDiscrete">
 <method>riskMeasuresDiscrete</method>


Is this method line still needed?

Do not needed anymore. removed.

wangcj05 · 2021-04-23T15:26:26Z

@alfoa This is ready for you to review. Several things to mention:

I have split the post-processor documents (not for all the postprocessors, but I think we should. If you agree, I can do it in another PR)
RavenOut PP is still exist in the manual, I think we do not have the RavenOut PP, and we should remove it, in the codes, tests, and documents.

wangcj05 · 2021-04-29T04:27:52Z

@alfoa Any comments for this PR?

moosebuild · 2021-05-04T22:33:05Z

Job Test qsubs on b7266a0 : invalidated by @alfoa

alfoa · 2021-05-04T22:34:43Z

Approved.

Yes the RavenOut should be removed.

moosebuild · 2021-05-04T23:13:16Z

Job Test OpenSUSE Leap 15 on b7266a0 : invalidated by @wangcj05

alfoa · 2021-05-04T23:24:14Z

Checklist passed...
Test OpenSUSE Leap 15 has still IT issue (@joshua-cogliati-inl do we have an eta for this?) but the tests have been run locally.

PR can be merged.

wangcj05 added 15 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

resolve comments

7458d53

postprocessor plugin API

dd437f2

wangcj05 added the Do Not Merge label Apr 13, 2021

wangcj05 requested review from mandd, alfoa and PaulTalbot-INL April 13, 2021 19:31

wangcj05 added 5 commits April 15, 2021 14:44

convert FT Importer PP

94eeb38

convert MCS Importer

8419f8f

enable both dict and xarray.Dataset for pp plugin

a2166f7

convert DataClassifier to use dict as input

63cdfad

convert RiskMeasures PP

4fe0f84

PaulTalbot-INL reviewed Apr 19, 2021

View reviewed changes

joshua-cogliati-inl reviewed Apr 19, 2021

View reviewed changes

framework/Models/PostProcessors/DataClassifier.py Outdated Show resolved Hide resolved

fix precheck

3310ab8

alfoa requested changes Apr 20, 2021

View reviewed changes

joshua-cogliati-inl mentioned this pull request Apr 20, 2021

Add Lagged Variable PostProcessor #1523

Merged

9 tasks

joshua-cogliati-inl reviewed Apr 21, 2021

View reviewed changes

Base automatically changed from wangc/convert_pp_to_interface to devel April 22, 2021 19:42

wangcj05 added 7 commits April 22, 2021 14:16

Merge branch 'devel' into wangc/pp_plugin_api_fy21_v3

59c3bb8

update validateDict

8d435d2

update validateDict

84bb6b6

resolve comments

6f5e2f7

fix MCImporter

58245a4

split PostProcessor document into separate files

42a6653

update RiskMeasureDiscrete PP document

25f3431

wangcj05 added RAVENv2.1 All tasks and defects that will go in RAVEN v2.1 and removed Do Not Merge labels Apr 23, 2021

wangcj05 added 2 commits April 27, 2021 12:50

Merge branch 'devel' into wangc/pp_plugin_api_fy21_v3

3cadea0

Merge branch 'devel' into wangc/pp_plugin_api_fy21_v3

b7266a0

alfoa approved these changes May 4, 2021

View reviewed changes

alfoa mentioned this pull request May 4, 2021

[TASK] PostProcessor Plugin #1451

Closed

10 tasks

alfoa merged commit 10eafae into devel May 4, 2021

alfoa deleted the wangc/pp_plugin_api_fy21_v3 branch May 4, 2021 23:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Postprocessor plugin APIs #1512

Postprocessor plugin APIs #1512

wangcj05 commented Apr 13, 2021 •

edited by alfoa

Loading

wangcj05 commented Apr 13, 2021

PaulTalbot-INL commented Apr 13, 2021

PaulTalbot-INL Apr 19, 2021

alfoa Apr 20, 2021

wangcj05 Apr 20, 2021

PaulTalbot-INL Apr 20, 2021 •

edited

Loading

wangcj05 Apr 23, 2021

alfoa Apr 20, 2021

wangcj05 Apr 23, 2021

alfoa Apr 20, 2021

wangcj05 Apr 23, 2021

alfoa Apr 20, 2021

PaulTalbot-INL Apr 20, 2021

wangcj05 Apr 23, 2021

alfoa Apr 20, 2021

alfoa Apr 20, 2021

wangcj05 Apr 23, 2021

alfoa Apr 20, 2021

wangcj05 Apr 20, 2021

alfoa Apr 20, 2021

wangcj05 Apr 23, 2021

alfoa Apr 20, 2021

wangcj05 Apr 23, 2021

alfoa Apr 20, 2021

PaulTalbot-INL Apr 20, 2021

wangcj05 Apr 20, 2021

joshua-cogliati-inl Apr 21, 2021 •

edited

Loading

wangcj05 Apr 23, 2021

wangcj05 commented Apr 23, 2021

wangcj05 commented Apr 29, 2021

moosebuild commented May 4, 2021

alfoa commented May 4, 2021

moosebuild commented May 4, 2021

alfoa commented May 4, 2021

Postprocessor plugin APIs #1512

Postprocessor plugin APIs #1512

Conversation

wangcj05 commented Apr 13, 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

wangcj05 commented Apr 13, 2021

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

PaulTalbot-INL Apr 20, 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

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

Choose a reason for hiding this comment

joshua-cogliati-inl Apr 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangcj05 commented Apr 23, 2021

wangcj05 commented Apr 29, 2021

moosebuild commented May 4, 2021

alfoa commented May 4, 2021

moosebuild commented May 4, 2021

alfoa commented May 4, 2021

wangcj05 commented Apr 13, 2021 •

edited by alfoa

Loading

PaulTalbot-INL Apr 20, 2021 •

edited

Loading

joshua-cogliati-inl Apr 21, 2021 •

edited

Loading