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.

Sign up for GitHub

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

Jump to bottom

New style tests closes #44 #47

Merged
merged 19 commits into from
Oct 27, 2020
Merged

New style tests closes #44 #47

merged 19 commits into from
Oct 27, 2020

Conversation

bbrelje
Copy link
Collaborator

@bbrelje bbrelje commented Oct 24, 2020

Purpose

This PR updates all unit tests to the new testflo standard and adds a little bit of new coverage for DVconstraints. There is also a small bugfix that gets the blocks tests to run (they didn't since scipy 1.3.0)

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)
  • [ X] 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
  • [ X] Maintenance update
  • Other (please describe)

Testing

All the old test data was ported over into the new file format so all the DVGeometry tests are producing the exact same results as before. Test coverage is significantly expanded and more maintainable going forward.

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
  • [X ] 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

@bbrelje bbrelje requested a review from a team as a code owner October 24, 2020 18:11
@bbrelje bbrelje requested review from joanibal and anilyil October 24, 2020 18:11
@bbrelje
Copy link
Collaborator Author

bbrelje commented Oct 24, 2020

This one is good to go @anilyil @joanibal

@bbrelje bbrelje changed the title Newtests New style tests closes #44 Oct 24, 2020
@bbrelje
Copy link
Collaborator Author

bbrelje commented Oct 25, 2020 •
edited
Loading

I added some new tests and unfortunately it looks like the test 9 (colinearity constraint) derivatives are producing different values on different architectures. It passes on my machine and one of the Docker images.

Our choices are:

  • deprecate (is anyone using it?) and remove the test
  • add a warning (use at your own risk) and remove the test
  • someone else fixes the bug and we comment out the test for now
  • someone else fixes the bug and we leave the test in, but ignore test 9 failures when accepting PRs

I don't have time to fix the underlying issue. (edit: I at least identified the underlying issue and opened a ticket)

@joanibal
Copy link
Collaborator

I vote for option 1. I don't see much value in maintaining features that aren't ever used.

Could you post something in slack to see if anyone can think of a reason it should not be deprecated?

@bbrelje
Copy link
Collaborator Author

bbrelje commented Oct 25, 2020 •
edited
Loading

Differences in compilers were causing some pygeo pointsets to have exactly and nearly zero values in different places which was causing the ColinearityConstraint and PlanarityConstraint to have spurious derivative values in the baseline case, which has a lot of true zero input values.

I will open an issue and we can decide later whether to accept the issue or deprecate

anilyil
anilyil reviewed Oct 26, 2020
View reviewed changes
setup.py Outdated Show resolved Hide resolved
anilyil
anilyil reviewed Oct 26, 2020
View reviewed changes
.travis.yml Show resolved Hide resolved
@anilyil
Copy link
Contributor

anilyil commented Oct 26, 2020

Other than those two points, the tests look good and they run fine on my installation. Do you want me to check anything else in the tests themselves?

@bbrelje
Copy link
Collaborator Author

bbrelje commented Oct 26, 2020

No, I don't have anything else in particular to review

This was linked to issues Oct 26, 2020
No Travis CI coverage of DVConstraints #25
Closed
Update tests #44
Closed
ewu63
ewu63 previously approved these changes Oct 27, 2020
View reviewed changes
ewu63
ewu63 reviewed Oct 27, 2020
View reviewed changes
Copy link
Collaborator

@ewu63 ewu63 left a comment

Choose a reason for hiding this comment

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

Actually, I would prefer if the tests have more descriptive names (along with the ref files), but I guess that's more of a "nice to have".

@bbrelje
Copy link
Collaborator Author

bbrelje commented Oct 27, 2020

Actually, I would prefer if the tests have more descriptive names (along with the ref files), but I guess that's more of a "nice to have".

Let's pull it in for now, I need to move on to some other tasks. Right now the test names match the DVGeo tests which are numbered sequentially

@ewu63
Copy link
Collaborator

ewu63 commented Oct 27, 2020

Yeah fair, I'm not actually a maintainer for pygeo so I'll wait for others to review.

@anilyil anilyil self-requested a review October 27, 2020 19:38
anilyil
anilyil previously approved these changes Oct 27, 2020
View reviewed changes
joanibal
joanibal previously approved these changes Oct 27, 2020
View reviewed changes
@joanibal joanibal dismissed stale reviews from anilyil, ewu63, and themself via e19f71b October 27, 2020 20:14
@joanibal joanibal requested review from joanibal and anilyil October 27, 2020 20:18
@joanibal
Copy link
Collaborator

Sorry @anilyil , @nwu63
I removed the mpi4py requirement in the setup.py file during the merge and had to put it back.
Unfortunately that caused your reviews to be dismissed.

@joanibal joanibal merged commit f8176b0 into mdolab:master Oct 27, 2020
@joanibal
Copy link
Collaborator

🍾 Thanks @bbrelje for your hard work on this

bernardopacini pushed a commit to bernardopacini/pygeo that referenced this pull request Mar 17, 2021
@bbrelje
New style tests closes mdolab#44 (mdolab#47)
9cdaff3 
Co-authored-by: Josh Anibal <joanibal@umich.edu>
Co-authored-by: Josh Anibal <joanibal+github@umich.edu>
anilyil added a commit to anilyil/pygeo that referenced this pull request Sep 24, 2022
@anilyil
added pyspline solver test. this should fail until the pyspline pr md…
3f20635 
…olab#47 is merged. Updated the input file archive to include sabet's fuselage FFD
marcomangano pushed a commit that referenced this pull request Oct 5, 2022
@anilyil
Added coordinate transformations and a few fixes (#157)
022e6b2 
* checkpoint

* fix typo in docstring

* several fixes related to use cases with a symmetry plane

* fixed some integer divisions

* added the optional coordinate transformation routines

* bug fix in the section local dv class

* editing the symmetry fix a bit

* clarified comments

* too many typo fixes. added test for the coord xfer feature

* format and lint. changed "dir" argument to "mode"

* added pyspline solver test. this should fail until the pyspline pr #47 is merged. Updated the input file archive to include sabet's fuselage FFD

* flake fix

* minor tolerance adjustment on ffd generation test. this fails on my laptop with 1e-15 rel tolerance. the error is at 1.1e-15. Moved the tolerance to 1e-14, which should be plenty accurate

* addressing todos

* minor rewording

* addressing comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eirikurj eirikurj eirikurj left review comments

@joanibal joanibal joanibal approved these changes

@ewu63 ewu63 ewu63 approved these changes

@anilyil anilyil Awaiting requested review from anilyil

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update tests No Travis CI coverage of DVConstraints
5 participants
@bbrelje @joanibal @anilyil @ewu63 @eirikurj