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

Update a few libraries #1971

Merged
merged 4 commits into from
Sep 29, 2022
Merged

Conversation

joshua-cogliati-inl
Copy link
Contributor

@joshua-cogliati-inl joshua-cogliati-inl commented Sep 19, 2022


Pull Request Description

What issue does this change request address?

#1972

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

Updates scikit-learn, statsmodels and ray
Removes using some deprecated python features.


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.

@joshua-cogliati-inl joshua-cogliati-inl changed the title Update a few Update a few libraries Sep 19, 2022
@moosebuild
Copy link

Job Mingw Test on 2a3658a : invalidated by @joshua-cogliati-inl

failed in fetch

@PaulTalbot-INL
Copy link
Collaborator

Hm, some of the regolds look like notable changes to me. Does anyone know how "analytic" or not those gold files were?

@joshua-cogliati-inl
Copy link
Contributor Author

Hm, some of the regolds look like notable changes to me. Does anyone know how "analytic" or not those gold files were?

The clustering/datamining ones probably are not very analytic.

@joshua-cogliati-inl
Copy link
Contributor Author

Tests affected:

  • tests/framework/PostProcessors/DataMiningPostProcessor/DimensionalityReduction/SpectralEmbedding
  • tests/framework/PostProcessors/LimitSurface/testLimitSurfaceIntegralPPWithBoundingError
  • tests/framework/PostProcessors/TemporalDataMiningPostProcessor/Clustering/MiniBatchKMeans

@joshua-cogliati-inl
Copy link
Contributor Author

Comments on differences:

The tests/framework/PostProcessors/DataMiningPostProcessor/DimensionalityReduction/SpectralEmbedding is mostly due to a few points being fairly different.
The biggest difference is the 10th number:

4.9 3.1 1.5 0.1 0 10 0.0283713757382 -0.016950871197

versus:

4.9 3.1 1.5 0.1 0 10 0.0288232499833 -0.015954529555

The tests/framework/PostProcessors/LimitSurface/testLimitSurfaceIntegralPPWithBoundingError differences are solely due to the EventProbability_err being different. I am wondering if this is being calculated by subtracting two numbers that very close.
Original Windows:
0.01365
Original Other:
0.01371875
New Windows:
0.0138
New Linux:
0.01388125
New Mac:
0.013725

The tests/framework/PostProcessors/TemporalDataMiningPostProcessor/Clustering/MiniBatchKMeans look quantitatively similar:
centroid_0

@wangcj05 or @PaulTalbot-INL any more questions?

@joshua-cogliati-inl
Copy link
Contributor Author

I think EventProbability_err comes from:
loadDict[self.computationPrefix+"_err"] = np.full(len(lms), boundError)
in ravenframework/Models/PostProcessors/LimitSurfaceIntegral.py:

  def collectOutput(self, finishedJob, output):
    """
      Function to place all of the computed data into the output object
      @ In, finishedJob, JobHandler External or Internal instance, A JobHandler object that is in charge of running this post-processor
      @ In, output, dataObjects, The object where we want to place our computed results
      @ Out, None
    """
    evaluation = finishedJob.getEvaluation()
    pb, boundError = evaluation[1]
    lms = evaluation[0][0]
    if output.type == 'PointSet':
      # we store back the limitsurface
      dataSet = lms.asDataset()
      loadDict = {key: dataSet[key].values for key in lms.getVars()}
      loadDict[self.computationPrefix] = np.full(len(lms), pb)
      if self.computeErrrorBounds:
        if self.computationPrefix+"_err" not in output.getVars():
          self.raiseAWarning('ERROR Bounds have been computed but the output DataObject does not request the variable: "', self.computationPrefix+"_err", '"!')
        else:
          loadDict[self.computationPrefix+"_err"] = np.full(len(lms), boundError)
      output.load(loadDict,'dict')
    else:
      self.raiseAnError(Exception, self.type + ' accepts PointSet only')

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.

Changes are good, I just have one question about ray. @joshua-cogliati-inl

@@ -60,7 +60,7 @@ Note all install methods after "main" take
<nomkl os='linux' skip_check='True'/>
<numexpr os='linux'/>
<cmake skip_check='True' optional='True'/>
<ray source="pip" pip_extra="[default]">1.12</ray>
<ray source="pip" pip_extra="[default]">1.13</ray>
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is a ray 2.0 release, do you have a chance to take a look? I saw there was some fix for hanging jobs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will make another branch and see what happens if we switch to ray 2.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It worked, so I switched this pull request to use ray 2.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, ray 2.0 seems to be causing random failures in InternalParallel tests.

@moosebuild
Copy link

Job Test Ubuntu 18 PIP on f28fa11 : invalidated by @joshua-cogliati-inl

FAILED: Failed tests/framework/InternalParallelTests/ROMscikit

@moosebuild
Copy link

Job Test Ubuntu 18-2 Python 3 on f28fa11 : invalidated by @joshua-cogliati-inl

FAILED: Failed tests/framework/ensembleModelTests/testEnsembleModelNonLinearParallel

@moosebuild
Copy link

Job Test Ubuntu 20-2 Optional on 2a3658a : invalidated by @joshua-cogliati-inl

FAILED: Diff tests/framework/PostProcessors/LimitSurface/testLimitSurfacePostProcessor

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.

changes are good.

@wangcj05
Copy link
Collaborator

Checklist is good. Test are green. PR can be merged.

@wangcj05 wangcj05 merged commit 4d550ae into idaholab:devel Sep 29, 2022
@wangcj05
Copy link
Collaborator

@joshua-cogliati-inl Could you send out an email to user group since there are libraries update, and the users need to update their libraries also.

@joshua-cogliati-inl joshua-cogliati-inl deleted the update_a_few branch September 29, 2022 15:37
@joshua-cogliati-inl joshua-cogliati-inl mentioned this pull request Sep 29, 2022
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants