Skip to content

Feature scramjet#235

Merged
WallyMaier merged 8 commits intodevelopfrom
feature-scramjet
Apr 23, 2018
Merged

Feature scramjet#235
WallyMaier merged 8 commits intodevelopfrom
feature-scramjet

Conversation

@WallyMaier
Copy link
Contributor

Pull Request Checklist

  • Merge in the latest Develop branch changes to your branch
  • Remove .pyc files from your repository:
    • Linux/Mac
      • find . -name *.pyc -delete
    • Windows
      • del /S *.pyc
  • Run automatic regression and makes sure everything is passing
  • Did you make a new regression test that covers your new code?
  • Did you update your docstrings?
  • Did you update your headers to include your name and date?
  • Do a final compare with the Develop branch to be sure what you're changing

@coveralls
Copy link

coveralls commented Apr 18, 2018

Coverage Status

Coverage increased (+0.5%) to 63.825% when pulling cf77836 on feature-scramjet into ba05f17 on develop.

Copy link
Contributor

@timdmacdo timdmacdo left a comment

Choose a reason for hiding this comment

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

See comments. Changes should all be relatively minor.

SECTION
#Xle Yle Zle Chord Ainc Nspanwise Sspace
32.4164746575 1.54 7.777 0.95 0.0
32.4164746575 0.0 7.777 0.95 0.0
Copy link
Contributor

Choose a reason for hiding this comment

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

This file seems unrelated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So according to matt...this files is worthless, it is generated everytime the regression is run

Copy link
Contributor

Choose a reason for hiding this comment

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

My impression was that it was supposed to be checked to make sure it was consistent. That doesn't seem to be true here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Make this an issue for AVL coverage

@@ -0,0 +1,244 @@
# sramjet_network.py
Copy link
Contributor

Choose a reason for hiding this comment

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

"sramjet"

conditions = SUAVE.Analyses.Mission.Segments.Conditions.Aerodynamics()

# freestream conditions
EVAL = conditions.freestream
Copy link
Contributor

Choose a reason for hiding this comment

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

variables should be lower case

conditions_sizing = SUAVE.Analyses.Mission.Segments.Conditions.Aerodynamics()

# freestream conditions
SIZE = conditions_sizing.freestream
Copy link
Contributor

Choose a reason for hiding this comment

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

variables should be lower case

scramjet.thrust = thrust

#size the ramjet
scramjet_sizing(scramjet,6.5,20000.0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Identify hard coded values

Copy link
Contributor

Choose a reason for hiding this comment

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

Still needed

mdot_core = mdhc*np.sqrt(Tref/total_temperature_reference)*(total_pressure_reference/Pref)

# computing the dimensional thrust
#computing the dimensional thrust
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I like the space. I think this will be an ongoing battle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So do I, i dont know why I changed it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh...cause thats how the rest of the script was written.

fuel_flow_rate = np.fmax(0.1019715*FD2*TSFC/3600.,a) #use units package for the constants
# computing the power
fuel_flow_rate = np.fmax(0.1019715*FD2*TSFC/3600,a) #use units package for the constants
Copy link
Contributor

Choose a reason for hiding this comment

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

These numbers should be replaced so that the source is clear. In general though you should leave the decimal. This is good practice to avoid issues with integers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So add 1/g and unit conversion for the 3600?

Copy link
Contributor

Choose a reason for hiding this comment

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

That would work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left it how it is for now, as I did not alter/use this portion of code. I used evaluate stream thrust.

Copy link
Contributor

Choose a reason for hiding this comment

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

Make this an issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Assumptions:

Source:
Heiser, William H., Pratt, D. T., Daley, D. H., and Unmeel, B. M.,
Copy link
Contributor

Choose a reason for hiding this comment

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

Provide relevant chapters/pages

Copy link
Contributor

Choose a reason for hiding this comment

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

Resolved

"""

# Calculate wave angle
MU = np.arcsin(1./M0)
Copy link
Contributor

Choose a reason for hiding this comment

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

variables should be lower case if there is no compelling reason otherwise (i.e. CL v cl)

conditions.freestream.density = np.atleast_1d(rho)
conditions.freestream.dynamic_viscosity = np.atleast_1d(mu)
conditions.freestream.gravity = np.atleast_1d(9.81)
conditions.freestream.isentropic_expansion_factor = np.atleast_1d(scramjet.working_fluid.compute_gamma(T,p))
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is a common oversight in other parts of the code, but please provide a not hard coded value for gravity.

Copy link
Contributor

Choose a reason for hiding this comment

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

Still needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@WallyMaier WallyMaier merged commit 8408ac6 into develop Apr 23, 2018
@WallyMaier WallyMaier deleted the feature-scramjet branch September 24, 2018 16:39
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.

3 participants