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 reference to paper for OAS integration work #33

Merged
merged 4 commits into from
Jan 24, 2022

Conversation

eytanadler
Copy link
Collaborator

@eytanadler eytanadler commented Jan 22, 2022

Purpose

Added a short note on the new OAS work with a bibtex citation to the paper and updated other outdated information on the readme.

Type of change

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

  • Documentation update

Testing

Explain the steps needed to test the new code to verify that it does indeed address the issue and produce the expected behavior.

Checklist

Put an x in the boxes that apply.

  • 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

@eytanadler eytanadler requested review from a team, bbrelje, kanekosh, bernardopacini and hajdik and removed request for a team, bbrelje and kanekosh January 22, 2022 00:05
@coveralls
Copy link

coveralls commented Jan 22, 2022

Pull Request Test Coverage Report for Build 1740968238

  • 2 of 3 (66.67%) changed or added relevant lines in 2 files are covered.
  • 22 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.4%) to 87.428%

Changes Missing Coverage Covered Lines Changed/Added Lines %
openconcept/analysis/openaerostruct/drag_polar.py 1 2 50.0%
Files with Coverage Reduction New Missed Lines %
openconcept/analysis/openaerostruct/drag_polar.py 22 92.2%
Totals Coverage Status
Change from base Build 1730143752: -0.4%
Covered Lines: 5306
Relevant Lines: 6069

💛 - Coveralls

@eytanadler eytanadler requested a review from a team as a code owner January 22, 2022 00:33
bernardopacini
bernardopacini previously approved these changes Jan 24, 2022
Copy link
Collaborator

@bernardopacini bernardopacini left a comment

Choose a reason for hiding this comment

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

This looks good to me, although we also talked about this offline and I mentioned the approach of including this in the README instead.

Is this functionality that is not a part of OpenConcept / OAS and unique to this script, or is it a general OpenConcept functionality? If it is the latter, I would recommend including it in the README so that it is clearly visible when someone visits the repo and wants to utilize this work.

@eytanadler eytanadler changed the title Added reference to paper in OAS integration work Added reference to paper for OAS integration work Jan 24, 2022
@eytanadler eytanadler added the documentation This is related to documentation label Jan 24, 2022
bernardopacini
bernardopacini previously approved these changes Jan 24, 2022
Copy link
Collaborator

@bernardopacini bernardopacini left a comment

Choose a reason for hiding this comment

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

Looks good to me! You can also keep the citation in the script (in addition to the README) if you'd like. My suggestion wasn't meant to be exclusive. But, the PR is good to go by me.

@eytanadler
Copy link
Collaborator Author

Ah ok I'll add it back then. Before this is merged, I'm also going to see if I can figure out some of the warnings in the tests.

Copy link
Collaborator

@bernardopacini bernardopacini left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@eytanadler eytanadler merged commit 697fb9c into mdolab:master Jan 24, 2022
@eytanadler eytanadler deleted the OAS_bib branch January 24, 2022 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation This is related to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants