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

Added new OpenVSP wrapper #56

Merged
merged 108 commits into from
Mar 1, 2021
Merged

Added new OpenVSP wrapper #56

merged 108 commits into from
Mar 1, 2021

Conversation

anilyil
Copy link
Contributor

@anilyil anilyil commented Jan 16, 2021

Purpose

This PR adds the new OpenVSP wrapper. The old one used file i/o. This one does everything in memory.

Type of change

  • New feature (non-breaking change which adds functionality)

Testing

I have created a few tests, and will send them to @bbrelje. I will work with him to add these to pygeo tests.

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

….gitignore file. Added DVGeometryMulti to the init file.
…f components to consider, rather than the full set of components.
joanibal
joanibal previously approved these changes Feb 1, 2021
@ewu63
Copy link
Collaborator

ewu63 commented Feb 16, 2021

@bbrelje is the docker image with OpenVSP dependencies blocking this? If so I can try to work with Eirikur to get that sorted so we can merge this.

@bbrelje
Copy link
Collaborator

bbrelje commented Feb 17, 2021

@nwu63 the ESP/VSP image is blocking right now, yes

@ewu63
Copy link
Collaborator

ewu63 commented Feb 20, 2021

@bbrelje FYI the OpenVSP/ESP packages are now in deps

@bbrelje
Copy link
Collaborator

bbrelje commented Feb 26, 2021

@anilyil please merge in my tests from my fork: https://github.com/bbrelje/pygeo-1/tree/vsp so we can get this PR closed out

@anilyil
Copy link
Contributor Author

anilyil commented Feb 26, 2021

@bbrelje @nwu63 I merged Ben's branch here. Let me know if I need to do anything else.

@ewu63
Copy link
Collaborator

ewu63 commented Feb 26, 2021

Oh no I guess we should merge the ESP one first so we get the STL for this? In the mean time should we add skipTest to these that depend on STL? Same with ESP tests too

@bbrelje
Copy link
Collaborator

bbrelje commented Feb 26, 2021

@nwu63 we gotta draw the line somewhere with these skiptests. numpy-stl is required when installing the optional feature [testing] so I think it's reasonable for the tests to fail if numpy-stl is not present

but also not gonna die on this hill, so lmk how strongly you feel about that

bbrelje
bbrelje previously approved these changes Feb 26, 2021
@ewu63
Copy link
Collaborator

ewu63 commented Feb 26, 2021

@nwu63 we gotta draw the line somewhere with these skiptests. numpy-stl is required when installing the optional feature [testing] so I think it's reasonable for the tests to fail if numpy-stl is not present

but also not gonna die on this hill, so lmk how strongly you feel about that

Yeah I guess this is required for testing, so what we have is fine. However, at this point why don't we just include STL as a required package (not under testing)? It's readily available from PyPI and installation is easy, so that might be easier than trying to manage multiple dependency sets. I'm also fine either way.

@ewu63
Copy link
Collaborator

ewu63 commented Feb 26, 2021

@bbrelje See the Azure tests -- both of the Ubuntu images failed on one test, with atol of 4E-9 and 9E-7 against target atol of 1E-16. I see there's an rtol of 1 but because the reference value is exactly 0, the rtol is pretty useless there. Do we want to adjust the tolerance? What's the expected accuracy here?

Ubuntu builds are seeing slight diff between parallel and serial FD
@marcomangano
Copy link
Contributor

A single test is failing because the relative tolerance on a 0.0 ref value goes to inf

AssertionError: 
Not equal to tolerance rtol=1, atol=1e-06
Failed value for: %s_grad_error
Mismatched elements: 2 / 63 (3.17%)
Max absolute difference: 2.67608158e-06
Max relative difference: inf
 x: array([ 0.000000e+00,  0.000000e+00,  0.000000e+00,  0.000000e+00,
        0.000000e+00,  0.000000e+00,  0.000000e+00,  0.000000e+00,
        0.000000e+00,  0.000000e+00,  0.000000e+00,  0.000000e+00,...
 y: array(0.)

Is there a way to go around this in testflo?

@ewu63
Copy link
Collaborator

ewu63 commented Mar 1, 2021

Yeah the assert we use is a wrapper on numpy.assert_allclose, which basically checks for atol when the reference value for rtol is zero. This is exactly why we use both atol and rtol for testing, in case the reference value is very small.

Try another approach to handle the nonrepeatable parallel FD error on Ubuntu images
Relax the tolerance on the parallel FD test, reluctantly
@bbrelje
Copy link
Collaborator

bbrelje commented Mar 1, 2021

@joanibal let's merge this - tests are passing now and opened an issue for the underlying problem

@ewu63 ewu63 merged commit 79fd7e7 into mdolab:master Mar 1, 2021
bernardopacini pushed a commit to bernardopacini/pygeo that referenced this pull request Mar 17, 2021
Co-authored-by: joanibal <joanibal@umich.edu>
Co-authored-by: Ben Brelje <bbrelje@umich.edu>
Co-authored-by: Neil Wu <602725+nwu63@users.noreply.github.com>
Co-authored-by: Ben Brelje <benjamin.brelje@gmail.com>
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.

None yet

5 participants