Skip to content
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

Added the capability to specify time interval and removed function to… #1830

Merged
merged 8 commits into from
Jun 15, 2022

Conversation

yoshiurr-INL
Copy link
Collaborator

@yoshiurr-INL yoshiurr-INL commented May 13, 2022

… postprocess more than one feature and target set per step


Pull Request Description

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

close #1829

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

Allows users to specify time intervals in ratio terms of the full time interval (pivot parameter). To prevent confusion, the capability to postprocess multiple pairs of features and targets in one single step has been deprecated. To postprocess multiple time intervals, the user will now have to create a new step and add it to the sequence.


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.

… postprocess more than one feature and target set per step
Uploading required test files that were not addable during local branch merging.
@moosebuild
Copy link

Job Mingw Test on 95a2587 : invalidated by @yoshiurr-INL

Random issue

Copy link
Collaborator

@wangcj05 wangcj05 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to keep the capacities to compute multiple feature-target pairs in the single post-processors. @yoshiurr-INL Please let me know what do you think.

@yoshiurr-INL
Copy link
Collaborator Author

I considered reenabling the multiple feature-target pairs but realized this would essentially require the user to create multiple DataObjects and OutStreams anyway due to potentially using different pivot parameters depending on how many data points is provided from the feature and target.

If we recall, 'pivotParameterFeature' and 'pivotParameterTarget' are utilized for this postprocessor. If the grids are nonuniform, it is possible for one transient to use 'pivotParameterFeature' and for another transient to use 'pivotParameterTarget' due to the auto-selection algorithm implemented to choose coarser grids and have the finer data interpolate/extrapolate.

Then, the pivot parameter for the output DataObject cannot be described using a single pivot parameter. To avoid this risk, I propose to keep the current changes.

For the suggestion to add an attribute to provide the user the choice to use ratio or raw time values, I agree that should be an option and will make changes ASAP.

@yoshiurr-INL
Copy link
Collaborator Author

The capability to use ratios and raw values is now available.

@moosebuild
Copy link

Job Test qsubs sawtooth on 7c94021 : invalidated by @yoshiurr-INL

Restart Sawtooth tests

Copy link
Collaborator

@wangcj05 wangcj05 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yoshiurr-INL I have some comments for your consideration.

doc/user_manual/PostProcessors/Validation.tex Outdated Show resolved Hide resolved
doc/user_manual/PostProcessors/Validation.tex Outdated Show resolved Hide resolved
doc/user_manual/PostProcessors/Validation.tex Outdated Show resolved Hide resolved
doc/user_manual/PostProcessors/Validation.tex Outdated Show resolved Hide resolved
doc/user_manual/PostProcessors/Validation.tex Outdated Show resolved Hide resolved
ravenframework/Models/PostProcessors/Validations/PPDSS.py Outdated Show resolved Hide resolved
ravenframework/Models/PostProcessors/Validations/PPDSS.py Outdated Show resolved Hide resolved
ravenframework/Models/PostProcessors/Validations/PPDSS.py Outdated Show resolved Hide resolved
@moosebuild
Copy link

Job Test qsubs sawtooth on d444b2f : invalidated by @yoshiurr-INL

Restart Sawtooth tests

2 similar comments
@moosebuild
Copy link

Job Test qsubs sawtooth on d444b2f : invalidated by @yoshiurr-INL

Restart Sawtooth tests

@moosebuild
Copy link

Job Test qsubs sawtooth on d444b2f : invalidated by @yoshiurr-INL

Restart Sawtooth tests

@yoshiurr-INL yoshiurr-INL requested a review from wangcj05 May 31, 2022 18:41
Copy link
Collaborator

@wangcj05 wangcj05 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some minor comments for you to consider.

it will output an error.
\item \xmlNode{Targets}, \xmlDesc{comma separated string, required field}, specifies the names of the targets. Make sure the feature data are normalized by a nominal value. \nb Each target is paired with a feature listed in xml node \xmlNode{Features}.
To enable user defined time interval selection, this postprocessor will only consider the first feature name provided. If user provides more than one,
itt will output an error.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

itt --> it

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Comment on lines 150 to 153
\item \xmlNode{Options}
\begin{itemize}
\item \xmlNode{pivotParameter}
\end{itemize}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need these lines?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not need these. Deleted.

Comment on lines 65 to 66
separateFeatureDataInput.addParam("type", InputTypes.StringType, descr="""Specifies what type of user defined time intervals have been selected from
the feature pivot parameter. Options are currently `ratio' or `raw_values'""")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use raw string descr = r""" .... """, and change "raw_values" --> raw\_values

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed.

Comment on lines 73 to 74
separateTargetDataInput.addParam("type", InputTypes.StringType, descr="""Specifies what type of user defined time intervals have been selected from
the target pivot parameter. Options are currently `ratio' or `raw_values'""")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar to above comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed.

@moosebuild
Copy link

Job Test qsubs sawtooth on 08e843a : invalidated by @yoshiurr-INL

Restart Sawtooth tests

@moosebuild
Copy link

Job Test qsubs sawtooth on 08e843a : invalidated by @wangcj05

@wangcj05
Copy link
Collaborator

PR checklist is satisfied.

@wangcj05 wangcj05 merged commit 2ef0a74 into devel Jun 15, 2022
@wangcj05 wangcj05 deleted the yoshrk-dss-individual-phase-analysis branch June 15, 2022 17:32
@wangcj05 wangcj05 added the RAVENv2.2 for RAVENv2.2 Release label Jun 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RAVENv2.2 for RAVENv2.2 Release Ready To Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TASK] Update DSS Postprocessor to accept user designated time intervals
3 participants