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

Bug fixed in load transfer scheme #318

Merged
merged 6 commits into from
Jun 14, 2020

Conversation

kanekosh
Copy link
Contributor

Purpose

This PR is to fix the issue #314. The load transfer scheme was fixed so that it is now consistent with the formulas presented in the journal paper Jasa2018a. Further details about the bug can be found in #314.
Once approved, a new version 2.2.1 should be released.

I updated the followings:

  • Load transfer computation and its partial derivatives
  • Reference values of all aerostructural regression tests
  • Documentation (not related to the bug): n2 diagram from the latest OpenMDAO 3.1.1

Type of change

  • Bugfix (non-breaking change which fixes an issue)
  • Documentation update

Testing

  • To verify the new load transfer implementation, Sham and I performed mesh refinement studies that compare the new and old codes. We confirmed that differences in both AS analysis and optimization are small enough for reasonably fine meshes (details in Possible bug in load transfer scheme #314).
  • Partial derivatives implementation was verified by the load transfer unit test, which calls OpenMDAO's check_partials().

Checklist

  • 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

@kanekosh kanekosh requested review from ewu63 and shamsheersc19 June 12, 2020 20:44
@kanekosh kanekosh requested a review from a team as a code owner June 12, 2020 20:44
@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 95.877% when pulling 0e71868 on kanekosh:fix_load_transfer into c752bd4 on mdolab:master.

@ewu63
Copy link
Collaborator

ewu63 commented Jun 13, 2020

I don't have a lot of experience with this stuff so I'll defer to Sham and John. For that one failed test, is it related to this PR? I'm wondering if we should retrain the reference value instead of just relaxing the tolerance.

@kanekosh
Copy link
Contributor Author

Yes, the failed test was about this PR. The relative error was slightly above 1.e-5 for one of the optimization tests. Though I haven't confirmed, I think it was because of the version difference of a scipy optimizer between my local and Travis build.

@ewu63
Copy link
Collaborator

ewu63 commented Jun 14, 2020

Oh sorry I got confused, I thought you only changed the tolerance, but in fact you had already re-trained the values, but it somehow failed on Travis, due to some discrepancy between Travis and your local machine. Understood, I'm fine with this PR then.

@johnjasa and @shamsheersc19 do you feel that this deserves a minor version increment instead of a bugfix release?

@shamsheersc19 shamsheersc19 linked an issue Jun 14, 2020 that may be closed by this pull request
@shamsheersc19
Copy link
Contributor

@johnjasa and @shamsheersc19 do you feel that this deserves a minor version increment instead of a bugfix release?

@nwu63 I think this deserves a minor version increment.

@shamsheersc19 shamsheersc19 merged commit eb9d2b4 into mdolab:master Jun 14, 2020
@ewu63
Copy link
Collaborator

ewu63 commented Jun 14, 2020

@nwu63 I think this deserves a minor version increment.

If that is the case, then please increment the version and make a new release. I think @kanekosh should do this since he did the PR. Feel free to message me if you have any questions on how you should do this.

@shamsheersc19
Copy link
Contributor

If that is the case, then please increment the version and make a new release. I think @kanekosh should do this since he did the PR. Feel free to message me if you have any questions on how you should do this.

I misunderstood the versioning terminology. Proceeding with a patch release. Thanks!

@kanekosh kanekosh deleted the fix_load_transfer branch June 15, 2020 17:38
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.

Possible bug in load transfer scheme
4 participants