-
Notifications
You must be signed in to change notification settings - Fork 135
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
Add Lagged Variable PostProcessor #1523
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.
The interfacePostprocessor here should be converted to be a normal PostProcessor (please talk to @wangcj05 about the new structure)
variables. For example, if there a variable price that is set hourly, | ||
than new variable called price\_prev\_hour could be set by using a | ||
delay of -1. This can be useful for training a ROM or other data | ||
analysis. |
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.
Josh, please add an example here that precisely explains the Input DataObject and how the Output DataObject will get out. From the previous explanation I do not know how to use this PP and how to set up the output dataobject.
We need to be very verbose (with examples).
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.
Maybe in conjunction with the example you have below.
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.
Added a bit more here, and expanded the example to include the step and the data objects.
doc/user_manual/postprocessor.tex
Outdated
\begin{itemize} | ||
\item \xmlNode{delay}, \xmlDesc{empty}, a delay node with the following required parameters: | ||
\begin{itemize} | ||
\item \xmlAttr{original}, the variable to start with |
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.
This is not the format of the manual.
Example:
\item \xmlAttr{VAR}, \xmlDesc{required string attribute}, user-defined VARIABLE name etc etc
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.
Fixed.
doc/user_manual/postprocessor.tex
Outdated
\item \xmlAttr{original}, the variable to start with | ||
\item \xmlAttr{new}, the new variable to create | ||
\item \xmlAttr{steps}, the delay (if negative) or steps into the future (if positive) to use for the new variable (so -1 gives the previous, 1 gives the next) | ||
\item \xmlAttr{default}, the value to use for cases where there is no previous or next value. |
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 be verbose here... "there is no previous or next value". What do you mean? in terms of perfect match using the step? need clear explanation for the user
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.
Added: (such as the beginning when a negative delay is used, or the end when the delay is positive)
@@ -0,0 +1,91 @@ | |||
|
|||
""" | |||
This is to implement a delay or lagged parameters in a HistorySet |
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.
standard of docstring
|
||
class HistorySetDelay(PostProcessorInterfaceBase): | ||
""" | ||
Class to get lagged or delayed data out of a history set. |
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.
standard
inputSpecification = super().getInputSpecification() | ||
inputSpecification.setCheckClass(CheckInterfacePP("HistorySetDelay")) | ||
delayClass = InputData.parameterInputFactory("delay", InputTypes.StringType, | ||
descr="Adds a delay variable") |
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.
verbosity of the description
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.
those descriptions will be used for future automatic documentation. they must be of "manual grade"
|
||
def initialize(self): | ||
""" | ||
Method to initialize the HistorySetDelay |
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.
standard docstring
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.
Fixed.
<created>2021-04-19</created> | ||
<classesTested>InterfacedPostProcessor</classesTested> | ||
<description> | ||
Tests of the HistorySetDelay interfaced post-processor |
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 you improve the description answering to the following questions:
what do we test? What the expected outcomes? input and outputs? the model used?
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.
Fixed.
I have converted the "RiskMeasuresDiscrete" in PR: https://github.com/idaholab/raven/pull/1512/files |
The method run doesn't seem to be modified: |
The base class will be changed. Based on the new structure, the interfacePP class, the PPInterfaces, the PPInterfaceBaseClass are not needed anymore. There is only minor changes in the methods by using inheritance. |
Well, I attempted to convert it to a post processor in: f4244c6 |
This should be ready to review again. |
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.
Some minor comments.
tests/framework/PostProcessors/HistorySetDelay/test_historySetDelay.xml
Outdated
Show resolved
Hide resolved
<?xml version="1.0" ?> | ||
<Simulation verbosity="debug"> | ||
<TestInfo> | ||
<name>framework/Models/PostProcessors/HistorySetDelay</name> |
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.
This should be the test name, i.e. framework/PostProcessors.HistorySetDelay
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.
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.
Comments addressed
Checklist passed... Comments addressed.. Approved to merge. |
Merging... |
Pull Request Description
What issue does this change request address?
Closes #1522
What are the significant changes in functionality due to this change request?
This adds a new post processor.
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.