-
Notifications
You must be signed in to change notification settings - Fork 133
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
New representativity calculations post processor #2058
base: devel
Are you sure you want to change the base?
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.
minor comments
|
||
@article{Epiney1, | ||
title={A Systematic Approach to Inform Experiment Design Through Modern Modeling and Simulation Methods}, | ||
author={Epiney, A and Rabiti, C and Davis, C}, |
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 reference does not indicate if it is a journal of conf proceeding
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.
updated
@ In, None | ||
@ Out, stat, object, Basic Statistic PostProcessor Object | ||
""" | ||
from .. import factory as ppFactory # delay import to allow definition |
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.
should this import be moved to the beginning of the file?
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.
updated.
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 just reverted the change. It can not moved to the beginning of the file since the class has not been initialized yet. We need to first initialize the PP class then we can import the factory.
sensMatr[i, j] = sensDict[senName][0]* datasets[inpVar].meanValue / datasets[outVar].meanValue | ||
return sensMatr | ||
|
||
def _computeMoments(self, datasets, features, targets): |
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.
do we have something already developed that does the same functionality?
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 do have similar functions inside Basic Statistics PP. However, it seems there is a more general issue in the developed post-processor. The probability weight is not considered in some functions. For example, the _computeMoments
function. I will open an issue for this to allow this PP to accept probability weight in general.
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 issue #2196
@joshua-cogliati-inl It seems there is another issue with windows installation:
|
Job Mingw Test on 645644e : invalidated by @wangcj05 issue with installation, truststore is not installable because there are no viable options |
Job Test qsubs sawtooth on 645644e : invalidated by @wangcj05 |
Job Mingw Test on 645644e : invalidated by @wangcj05 |
Job Test qsubs sawtooth on 645644e : invalidated by @wangcj05 |
… modifying the input dataset
Job Test qsubs sawtooth on 6d7d77b : invalidated by @joshua-cogliati-inl failed in set python environment |
Job Test qsubs sawtooth on 548f9f7 : invalidated by @wangcj05 |
@mandd FYI, the tests are green for this PR. Please let me know if you have additional comments. |
Hi @mandd, I think this PR is ready for you to take another review. Could you take a look in the next two weeks? |
Job Test qsubs sawtooth on 548f9f7 : invalidated by @wangcj05 directory exists issue |
Job Test qsubs sawtooth on 548f9f7 : invalidated by @wangcj05 environment issue |
Job Test qsubs sawtooth on 548f9f7 : invalidated by @wangcj05 |
Job Test qsubs sawtooth on 548f9f7 : invalidated by @wangcj05 Diff tests/cluster_tests/test_mpiqsub_nosplit, not related to this PR |
@mandd @PaulTalbot-INL @dylanjm @yoshiurr-INL @GabrielSoto-INL Can any of you review this PR? Both Josh and I have contributions to this PR. |
Job Test Ubuntu 18 PIP on 4ca3eb7 : invalidated by @joshua-cogliati-inl restarted civet |
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.
Several requests and comments for review.
\item \xmlNode{Targets}, \xmlDesc{comma separated string, required field}, contains a comma separated list of | ||
targets. \nb Each target is paired with a feature listed in xml node \xmlNode{Features}. In this case, the | ||
number of targets should be equal to the number of features. | ||
\item \xmlNode{prototypeOutputs}, \xmlDesc{comma separated string, required field}, specifies the names of the prototype outputs. |
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.
Is this sufficient description to understand what we mean by prototype outputs? I don't feel like I have a good understanding, based solely on this description.
\begin{itemize} | ||
\item \xmlAttr{type}, \xmlDesc{optional string attribute}, options are `ratio' or `raw\_values'. The default is `ratio'. | ||
\end{itemize} | ||
\item \xmlNode{multiOutput}, \xmlDesc{string, required field}, to extract raw values for the HistorySet. The user must use ‘raw values’ for the full set of metrics’ calculations to be dumped. |
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 grammar here could be touched up. What does "to extract raw values from the History Set" mean?
three versions of PCM so far: `Static', `Snapshot', and `Tdep'. Static PCM is for static problem, and Snapshot PCM | ||
and Tdep PCM are for time-dependent problem. | ||
\paragraph{Representativity} | ||
The \textbf{Representativity} post-processor is one of three \textbf{Validation} post-processors, in fact there is a |
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 a period after "post-processors"? "in fact there is" is a strange statement after a comma.
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 description also seems a little focused on a software engineer's understanding of this interface. Can we make it more user-centric?
%form a good representation of a target model. Two, if a set of experiments can represent a target model and can | ||
%claim a full coverage of the design space and scenarios, and three, if the available set of experiments are not | ||
%enough to declare coverage what are the remaining experiments required in order to achieve full coverage and | ||
%increase the representativity/bias factor. |
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.
keep or remove this commented description?
@ In, UmesVar, np.array, variance covariance matrix of the measurables in the error space | ||
@ In, normalizedSen, np.array, the normalized sensitivity matrix | ||
@ Out, parTilde, np.array, the corrected parameters | ||
@ Out, parTildeVar, np.array, the variance covariance matrix of the corrected parameters (uncertainty in the corrected parameters) |
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.
it looks like this name changed
""" | ||
# Compute adjusted target #eq 71 | ||
UtarTilde = normalizedSenTar @ UparVar @ normalizedSenExp.T @ np.linalg.pinv(normalizedSenExp @ UparVar @ normalizedSenTar.T + UmesVar) @ Umes | ||
# back transform to parameters |
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.
Is this comment resolved?
@@ -187,7 +187,7 @@ def localFinalizeActualSampling(self, jobObject, model, myInput): | |||
@ Out, None | |||
""" | |||
if self.counter > 1: | |||
output = self.basicStatPP.run(self._targetEvaluation) | |||
output = self.basicStatPP._runLegacy(self._targetEvaluation) |
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.
Is this resolved?
<variable name="p2"> | ||
<distribution>dist2</distribution> | ||
</variable> | ||
<constant name="e1">2,-3</constant> |
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.
resolved?
<variable name="p2"> | ||
<distribution>dist2</distribution> | ||
</variable> | ||
<constant name="o1">2,-3</constant> |
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.
resolved?
Pull Request Description
What issue does this change request address? (Use "#" before the issue to link it, i.e., #42.)
Closes #1650 and #1546
This PR will add a representativity calculations post-processor. This post-processor can be used to calculation similarities between prototype and full scale models/experiments, perform input data adjustment, and uncertainty reductions for full scale models/experiments.
Future development will enable this post-processor to accept probability weight coming from other Samplers, such as grid/LHS. See #2196
What are the significant changes in functionality due to this change request?
Implements the representativity theory through bias factors for validation purposes
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.