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

Docs overhaul and refactor to organize code #41

Merged
merged 89 commits into from
Aug 10, 2022

Conversation

eytanadler
Copy link
Collaborator

@eytanadler eytanadler commented Aug 5, 2022

Purpose

This PR...

  • Introduces a new set of documentation with detailed tutorials, model explanations, and user guides
  • Reorganizes the source code directories to make more intuitive sense for users
  • Adds GHA checks for black and flake8
  • Adds Python versions 3.8, 3.9. and 3.10 to the test matrix
  • Removes the six dependency
  • Addresses Add more detailed docs #36.

Do not bump the version or format the code in this PR, do it in another so we can ignore the changes in git blame.

Type of change

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

  • Breaking change (non-backwards-compatible fix or feature)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Documentation update

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 a review from a team as a code owner August 5, 2022 12:27
@eytanadler eytanadler marked this pull request as draft August 5, 2022 12:27
@codecov
Copy link

codecov bot commented Aug 5, 2022

Codecov Report

Merging #41 (a58c4e8) into main (8a2fcb5) will decrease coverage by 0.21%.
The diff coverage is n/a.

❗ Current head a58c4e8 differs from pull request most recent head 003dac9. Consider uploading reports for the commit 003dac9 to get more accurate results

@@            Coverage Diff             @@
##             main      #41      +/-   ##
==========================================
- Coverage   87.49%   87.27%   -0.22%     
==========================================
  Files          47       47              
  Lines        6101     6099       -2     
==========================================
- Hits         5338     5323      -15     
- Misses        763      776      +13     
Impacted Files Coverage Δ
analysis/openaerostruct/aerostructural.py 90.84% <0.00%> (-8.26%) ⬇️
analysis/performance/solver_phases.py 99.55% <0.00%> (-0.01%) ⬇️
utilities/math/integrals.py 91.22% <0.00%> (ø)
analysis/performance/dymos_phases.py 0.00% <0.00%> (ø)
analysis/openaerostruct/drag_polar.py 98.66% <0.00%> (+6.14%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

Copy link

@lamkina lamkina left a comment

Choose a reason for hiding this comment

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

A summary of my comments besides typos in the docs:

  • It looks like OpenMDAO has an AddSubtractComp that is potentially a copy of the OpenConcept component.
  • from __future__ import division shows up a lot and from my understanding isn't necessary in python 3.
  • It might be useful to define a Constants module that collects any constant global variables used across the API

Copy link

@lamkina lamkina left a comment

Choose a reason for hiding this comment

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

There's one constant in the atmospherics module that can be moved into the constants folder and then I think this will be ready to rock.

lamkina
lamkina previously approved these changes Aug 10, 2022
Copy link

@lamkina lamkina left a comment

Choose a reason for hiding this comment

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

Great work @eytanadler. This looks ready to go!

Copy link
Contributor

@kanekosh kanekosh left a comment

Choose a reason for hiding this comment

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

Added comments to the dymos-related files. Other than those, looks good

@@ -168,7 +165,7 @@ def show_outputs(prob):
x_label = 'Range (nmi)'
y_labels = ['Altitude (ft)', 'Veas airspeed (knots)', 'Fuel used (lb)', 'Throttle setting', 'Vertical speed (ft/min)', 'Mach number', 'CL']
phases = ['climb', 'cruise', 'descent','reserve_climb','reserve_cruise','reserve_descent','loiter']
oc.plot_trajectory(prob, x_var, x_unit, y_vars, y_units, phases,
plot_trajectory(prob, x_var, x_unit, y_vars, y_units, phases,
x_label=x_label, y_labels=y_labels, marker='-',
plot_title='737-800 Mission Profile')

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we remove this file? It is a duplicate of B738.py - I checked the diff between B738_Dymos.py and B738.py and two were identical.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch, I'll remove it

J['fltcond|CL','weight'] = inputs['fltcond|cosgamma']*GRAV_CONST/inputs['fltcond|q']/inputs['ac|geom|wing|S_ref']
J['fltcond|CL','fltcond|q'] = - inputs['fltcond|cosgamma']*GRAV_CONST*inputs['weight'] / inputs['fltcond|q']**2 / inputs['ac|geom|wing|S_ref']
J['fltcond|CL','ac|geom|wing|S_ref'] = - inputs['fltcond|cosgamma']*GRAV_CONST*inputs['weight'] / inputs['fltcond|q'] / inputs['ac|geom|wing|S_ref']**2
J['fltcond|CL','fltcond|cosgamma'] = GRAV_CONST*inputs['weight']/inputs['fltcond|q']/inputs['ac|geom|wing|S_ref']

class DymosSteadyFlightODE(om.Group):
Copy link
Contributor

Choose a reason for hiding this comment

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

Other than DymosSteadyFlightODE, the classes in this file are duplicates of phases.py. Also, I believe this dymos component is not used anywhere, so we should either remove this file, or only keep DymosSteadyFlightODE and import other stuff from phases.py

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The DymosSteadyFlightODE has very few changes from the SteadyFlightPhase, so I don't think it's worth keeping. I'm getting rid of it too

@eytanadler eytanadler merged commit 1c407d9 into mdolab:main Aug 10, 2022
@eytanadler eytanadler deleted the docs_and_refactor branch August 10, 2022 23:56
@eytanadler eytanadler mentioned this pull request Aug 11, 2022
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

3 participants