Closes #673 #674

alfoa · 2018-06-04T20:17:05Z

Pull Request Description

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

Closes #673
Closes #695
Closes #583
Closes #697

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

Fixed the pickling and loading for DMD-based ROMs.
Added a test in the DMD
Fixed SingleRun prefix with HDF5 and modified a test
Fixed manual for optimizer
Fixed pivotParameter error for TimeDependent ROMs

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?

Checklist review by @wangc on July 20, 2018

wangcj05

In addition to the comment, I think we also need to add the unit tests for the marginal and inverse marginal distributions.

wangcj05 · 2018-06-04T22:43:27Z

crow/include/distributions/distributionNDInverseWeight.h

@@ -184,6 +184,15 @@ class BasicMultiDimensionalInverseWeight: public virtual BasicDistributionND
    return value;
  }

+  double marginal(double x, int dimension){


wangcj05 · 2018-06-04T22:45:54Z

crow/src/distributions/distributionNDCartesianSpline.cxx

@@ -156,14 +156,16 @@
      if (_cdf_provided){
        throwError("BasicMultiDimensionalCartesianSpline Distribution error: inverseMarginal calculation not available if CDF provided");
      }else{
-        value = _interpolator.splineCartesianInverseMarginal(f, dimension, 0.01);
+        value = _interpolator.splineCartesianInverseMarginal(f, dimension, 1.e-6);


Should we remove the coded value, and add a variable to control that?

yes this is the final plan

wangcj05 · 2018-06-04T22:46:33Z

crow/src/distributions/distributionNDCartesianSpline.cxx

      }
    }else
      throwError("BasicMultiDimensionalCartesianSpline Distribution error: CDF value for inverse marginal distribution is above 1.0");

    return value;
  }

+
+


remove extra line?

wangcj05 · 2018-06-04T22:55:05Z

framework/SupervisedLearning.py

@@ -3229,6 +3229,15 @@ def __init__(self,messageHandler,**kwargs):
    if self.pivotParameterID not in self.target:
      self.raiseAnError(IOError,"The pivotParameter "+self.pivotParameterID+" must be part of the Target space!")

+  def __setstate__(self, newstate):


newstate or newState?

alfoa · 2018-06-04T23:57:04Z

Comments addressed + added unit test

wangcj05

minor comment, the docstring of variable in marginalDistribution and inverseMarginalDistribuation, may need to add more informative statement, associate the variable id to the dimension in the nd distribution, which will be more clear for the future developer.

wangcj05 · 2018-06-05T03:53:08Z

The unit test that you created is failed:

framework/unit_tests/Distributions.Distributions: checking answer MultiVariate inverseMarginalDim1(0.5) 2999.99999881 != 3000.419
framework/unit_tests/Distributions.Distributions: checking answer MultiVariate inverseMarginalDim2(0.5) 2499.97185556 != 2500.044

You may need to update your checked values.

alfoa · 2018-06-05T04:02:14Z

ok fixed unit test and better docstrings

wangcj05 · 2018-06-05T04:32:20Z

tests/framework/unit_tests/Distributions/TestDistributions.py

    @ Out, None
  """
-  if abs(value - expected) > tol:
+  diff = abs(value - expected) if not relative else abs(value - expected)/expected


check expected!=0

wangcj05 · 2018-06-05T16:46:49Z

Tests are failed, how do you determine the checked value for the unit tests? are they depend on running systems?

alfoa · 2018-06-05T17:07:25Z

Well....I am simply pushing the value I get from the mac... The correct value should be 0.5 but unfortunately since the hard-coded discretizations and tolerances I am not able to get it.
I will relax the Multivariate peak to remove the wiggles of the spline.

moosebuild · 2018-06-05T20:41:12Z

Job Mingw Test on 805d030 : canceled by @alfoa

the machine is super slow

moosebuild · 2018-06-05T20:41:20Z

Job Mingw Test on 805d030 : invalidated by @alfoa

moosebuild · 2018-06-06T20:19:30Z

Job Mingw Test on 805d030 : invalidated by @alfoa

moosebuild · 2018-06-06T22:00:59Z

Job Mingw Test on 805d030 : invalidated by @PaulTalbot-INL

Invalidating due to civet restart with new libraries.

moosebuild · 2018-06-08T00:38:10Z

Job Mingw Test on 805d030 : invalidated by @alfoa

moosebuild · 2018-06-12T00:22:10Z

Job Mingw Test on 805d030 : invalidated by @alfoa

PaulTalbot-INL · 2018-06-12T13:56:37Z

For the record, since we have to force library versions on Windows manually, until #668 merges the libraries will not pass on other MRs. Just as a reminder.

moosebuild · 2018-07-18T10:49:45Z

Job Mingw Test on 805d030 : invalidated by @alfoa

…to alfoa/issue673

alfoa · 2018-07-20T16:15:28Z

@wangcj05 can you review this again? It should be green now

wangcj05

minor comments

wangcj05 · 2018-07-20T16:26:02Z

framework/Steps.py

      try:
        os.mkdir(currentWorkingDirectory)
      except OSError:
-        self.raiseAWarning('current working dir '+currentWorkingDirectory+' already exists, this might imply deletion of present files')
+        self.raiseAWarning('current working dir '+currentWorkingDirectory+' already exists, ' +
+                           'this might imply deletion of present files')


better to use , instead of +, but it is fine if not change it.

wangcj05 · 2018-07-20T16:27:45Z

framework/Steps.py

+              self.raiseAWarning('As prescribed in the input, trying to re-submit the job "'+
+                                 finishedJob.identifier+'". Trial '+
+                               str(self.failureHandling['jobRepetitionPerformed'][finishedJob.identifier]) +
+                               '/'+str(self.failureHandling['repetitions']))


ok here, but better to use ,

wangcj05 · 2018-07-20T16:28:01Z

framework/Steps.py

              self.failureHandling['jobRepetitionPerformed'][finishedJob.identifier] += 1
            else:
              #add run to a pool that can be sent to the sampler later
              self.failedRuns.append(copy.copy(finishedJob))
-              self.raiseAWarning('The job "'+finishedJob.identifier+'" has been submitted '+ str(self.failureHandling['repetitions'])+' times, failing all the times!!!')
+              self.raiseAWarning('The job "'+finishedJob.identifier+'" has been submitted '+
+                                 str(self.failureHandling['repetitions'])+' times, failing all the times!!!')


same as before

wangcj05 · 2018-07-20T16:30:13Z

framework/SupervisedLearning.py

@@ -2210,6 +2210,9 @@ def __init__(self,messageHandler,**kwargs):
    """
    supervisedLearning.__init__(self,messageHandler,**kwargs)
    name  = self.initOptionDict.pop('name','')
+    if 'pivotParameter' in self.initOptionDict:
+      # remove pivot parameter if present
+      self.initOptionDict.pop("pivotParameter")


I did not get here. You changed the way to retrieve the pivotParameter in the LearningGate, but here you start to remove it. why?

because the pivotParameter is needed internally for all the TimeDep ROMs, but not for the SKLearn ROMs., since we pass directly the dictionary into the SKL constructors (that complains in case a variable is not recognized (e.g. pivotParameter)

wangcj05 · 2018-07-20T17:01:19Z

This PR looks good to me. I will go through the checklist.

alfoa · 2018-07-20T17:46:54Z

Proposed email:

Dear users,
we recently realized that there was a mismatch between the user manual and the Code regarding the GradientBased optimizers.
The code was expecting the following two parameters:

gainGrowthFactor and gainShrinkFactor

but in the manual they were called gainGrowthSize and gainShrinkSize.

In addition:

the DMD-based ROMs were not serializable. This was not allowing the usage of the python parallelization with these ROMs.
the HDF5 databases were not usable with SingleRun steps.

The issues have been solved with the PR #674 and the modification is currently available in the devel branch

moosebuild · 2018-07-20T18:19:09Z

Job Test mac on 3796880 : invalidated by @alfoa

alfoa

approved

Closes #673

aac82cb

alfoa requested a review from wangcj05 June 4, 2018 20:17

alfoa added 2 commits June 4, 2018 15:09

removed leftover

8fe4f2d

added margina to crow

a79b63a

wangcj05 requested changes Jun 4, 2018

View reviewed changes

alfoa added 2 commits June 4, 2018 17:22

added wright

38f11ee

regolded tests

00c40bb

alfoa added 2 commits June 4, 2018 17:57

addressed wangcj05 coments

f99c634

unit test

303adca

wangcj05 approved these changes Jun 5, 2018

View reviewed changes

alfoa added 2 commits June 4, 2018 21:59

added relative tolerance

0c4fae8

improved docstrings

278724f

wangcj05 requested changes Jun 5, 2018

View reviewed changes

addressed Congjian's comments

33bbfb7

wangcj05 approved these changes Jun 5, 2018

View reviewed changes

relaxed multivariate test

805d030

wangcj05 approved these changes Jun 5, 2018

View reviewed changes

alfoa added 7 commits July 20, 2018 06:52

Closes #695

33632f7

added test to test the fix

1a7777c

Closes #583

9e287f9

added pivotparameter test

dbb5b77

modified tests for new pivotparameter strategy

0cb008e

Merge branch 'alfoa/issue695' of https://github.com/idaholab/raven in…

8d63364

…to alfoa/issue673

modified SIngleRun for setting a default prefix

f70dbeb

alfoa mentioned this pull request Jul 20, 2018

SINGLE RUN with HDF5 crash #695

Closed

10 tasks

wangcj05 reviewed Jul 20, 2018

View reviewed changes

modified gold file

3dd530c

wangcj05 approved these changes Jul 20, 2018

View reviewed changes

typo for the gainGrowthFactor

3796880

alfoa commented Jul 20, 2018

View reviewed changes

wangcj05 merged commit 6ff507c into devel Jul 20, 2018

alfoa deleted the alfoa/issue673 branch July 20, 2018 19:17

PaulTalbot-INL added a commit to PaulTalbot-INL/raven that referenced this pull request Jul 23, 2018

merged in devel after idaholab#674

5e9d90a

Closes #673 #674

Closes #673 #674

Conversation

alfoa commented Jun 4, 2018 • edited by wangcj05 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 left a 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

alfoa commented Jun 4, 2018

wangcj05 left a comment

Choose a reason for hiding this comment

wangcj05 commented Jun 5, 2018

alfoa commented Jun 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangcj05 commented Jun 5, 2018

alfoa commented Jun 5, 2018

moosebuild commented Jun 5, 2018

moosebuild commented Jun 5, 2018

moosebuild commented Jun 6, 2018

moosebuild commented Jun 6, 2018

moosebuild commented Jun 8, 2018

moosebuild commented Jun 12, 2018

PaulTalbot-INL commented Jun 12, 2018

moosebuild commented Jul 18, 2018

alfoa commented Jul 20, 2018

wangcj05 left a 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 commented Jul 20, 2018

alfoa commented Jul 20, 2018 • edited Loading

moosebuild commented Jul 20, 2018

alfoa left a comment

Choose a reason for hiding this comment

alfoa commented Jun 4, 2018 •

edited by wangcj05

Loading

alfoa commented Jul 20, 2018 •

edited

Loading