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

Removed PETSc #9

Merged
merged 1 commit into from
Feb 17, 2022
Merged

Removed PETSc #9

merged 1 commit into from
Feb 17, 2022

Conversation

sseraj
Copy link
Collaborator

@sseraj sseraj commented Feb 17, 2022

Purpose

I removed unnecessary PETSc inclusion in the default config files. Believe it or not, this was causing the ADflow, IDWarp, and pyAeroStructure complex tests to fail on the c7-intel image after we merged mdolab/pygeo#113.

Type of change

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (non-backwards-compatible fix or feature)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Documentation update
  • Maintenance update
  • Other (please describe)

Testing

Checklist

  • I have run flake8 and black to make sure the code adheres to PEP-8 and is consistently formatted
  • I have run unit and regression tests which pass locally with my changes
  • I have added new tests that prove my fix is effective or that my feature works
  • I have added necessary documentation

@sseraj sseraj requested a review from a team as a code owner February 17, 2022 21:16
@codecov
Copy link

codecov bot commented Feb 17, 2022

Codecov Report

Merging #9 (2c15414) into master (37e1464) will not change coverage.
The diff coverage is n/a.

❗ Current head 2c15414 differs from pull request most recent head 4550427. Consider uploading reports for the commit 4550427 to get more accurate results

Impacted file tree graph

@@           Coverage Diff           @@
##           master       #9   +/-   ##
=======================================
  Coverage   46.97%   46.97%           
=======================================
  Files           5        5           
  Lines        1816     1816           
=======================================
  Hits          853      853           
  Misses        963      963           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37e1464...4550427. Read the comment docs.

@yqliaohk yqliaohk merged commit e79b8ac into master Feb 17, 2022
@sseraj sseraj deleted the remove-petsc branch February 17, 2022 21:33
@eirikurj
Copy link
Contributor

@sseraj I did not look into the failed tests, but I am surprised that the PETSc config failed the tests. Even though you are not actually using PETSc, it should (in principle) be fine to link PETSc (assuming that was the issue), and how is that interfering with pygeo? For my information, can you provide more information on the issue?

@sseraj
Copy link
Collaborator Author

sseraj commented Feb 18, 2022

@eirikurj Unfortunately, I don't have a great explanation for why this was causing issues (and only on the c7-intel image). @nwu63 and I bisected code changes until we found that the recently added pySurf import in pyGeo was causing the problem. This combined with the fact that only tests involving complex PETSc were failing suggested removing PETSc from pySurf.

@eirikurj
Copy link
Contributor

Ok, no worries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants