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

Support latest PETSC #54

Merged
merged 9 commits into from
Jul 14, 2021
Merged

Support latest PETSC #54

merged 9 commits into from
Jul 14, 2021

Conversation

A-CGray
Copy link
Member

@A-CGray A-CGray commented Jul 12, 2021

Purpose

Fixing issues with the VecGetValues function so that idwarp works with newer versions of petsc, part of tackling https://github.com/mdolab/docker/issues/83.

We haven't figured out why, but in newer versions of petsc, VecGetValues expects a scalar rather than a single entry array for the index when retrieving a single value. I added some conditional compilation so that we use the correct inputs depending on the petsc version installed and therefore maintain backwards compatibility.

Type of change

What types of change is it?
Select the appropriate type(s) that describe this PR

  • 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

Testflo tests pass locally with petsc 3.12 and on docker image with 3.15

Checklist

Put an x in the boxes that apply.

  • 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

@A-CGray A-CGray requested a review from a team as a code owner July 12, 2021 18:41
@A-CGray A-CGray changed the title add idwarp.egg-info to gitignore Support latest PETSC Jul 12, 2021
@ewu63
Copy link
Collaborator

ewu63 commented Jul 12, 2021

Does the scalar argument not work for older version of PETSc?

@A-CGray
Copy link
Member Author

A-CGray commented Jul 12, 2021

Nope, I get this:

getSurfaceCoordinates.F90:22:31:

   22 |       call VecGetValues(Xs, 1, iStart+i-1, coordinates(i), ierr)
      |                               1
Error: Rank mismatch in argument ‘c’ at (1) (rank-1 and scalar)

@marcomangano
Copy link
Contributor

(I was getting extremely confused about the difference between this and #52 , then I looked at the branches - why was that necessary? The commit history looks the same)

@A-CGray
Copy link
Member Author

A-CGray commented Jul 12, 2021

I think I was being an idiot and couldn't figure out how to push directly to the modlab fork of idwarp so had to push to my own, that PR just merged my fork's update-petsc branch into the mdolab one, this PR is to merge those changes into master

@marcomangano
Copy link
Contributor

alright I see, and ofc you did not squash so to a reviewer they look like "double commits". No worries, looks fine!

ewu63
ewu63 previously approved these changes Jul 13, 2021
@A-CGray
Copy link
Member Author

A-CGray commented Jul 13, 2021

Hold back on merging for now, I think we can do this in a much simpler way

@A-CGray
Copy link
Member Author

A-CGray commented Jul 13, 2021

Was able to figure out how to retrieve all necessary values with a single call to VecGetValues this means we don't need to do all that conditional compilation, I kept it in verifyWarpDeriv though as there the call to VecGetValues is part of a much bigger loop that I wasn't confident messing with

@ewu63 ewu63 requested review from eirikurj and joanibal July 14, 2021 18:11
@eirikurj eirikurj merged commit f72fe45 into master Jul 14, 2021
@eirikurj eirikurj deleted the update-petsc branch July 14, 2021 22:14
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