Skip to content

LTO,PTF functionality and tests added #127

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

Open
wants to merge 67 commits into
base: main
Choose a base branch
from

Conversation

aditeyashukla
Copy link
Contributor

Updated functions for

  • odperf
  • LTO
  • printBADA

Added P3T3 function for both narrow body (CFM56) and wide body (GE90) using curve fitting

Added tests for all 3 functions

Also, added aircraft_type in aircraft struct

Copy link

codecov bot commented Feb 11, 2025

Codecov Report

Attention: Patch coverage is 70.00000% with 150 lines in your changes missing coverage. Please review.

Project coverage is 81.08%. Comparing base (181c5a0) to head (45f8c6b).

Files with missing lines Patch % Lines
src/mission/odperformance.jl 63.14% 129 Missing ⚠️
src/IO/output_LTO_perf.jl 86.66% 10 Missing ⚠️
src/IO/output_BADA.jl 86.88% 8 Missing ⚠️
src/IO/read_input.jl 83.33% 2 Missing ⚠️
src/misc/aircraft.jl 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #127      +/-   ##
==========================================
+ Coverage   79.04%   81.08%   +2.04%     
==========================================
  Files          84       80       -4     
  Lines       13650    13864     +214     
==========================================
+ Hits        10790    11242     +452     
+ Misses       2860     2622     -238     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rename to generate_BADA()

@argonaut22
Copy link
Contributor

To clarify my drowsy thoughts from earlier:

  • I think LTO.jl and printBADA.jl belong in IO/ as output_offdesign.jl: output_LTO_perf and output_BADA. Though there are calcs being done, they don't seem to affect the aircraft data and, thus, it's output
  • odperf does seem to affect the ac via tfcalc! and cdsum!. thus, makes sense in mission/. Does running odperf change the aircraft for subsequent evaluations of other missions? I'm not that familiar with this functionality
  • I still don't like aircraft_type as a top-level option since it's unused except for this one function. Would prefer a required parameter like odperf!(ac, aircraft_type("narrow body")). If we must have it, then please just use Strings. we evolved past the need for Int flags

elseif lowercase(aircraft_type) == "regional aircraft"
return :regional
else
return :narrow
Copy link
Contributor

Choose a reason for hiding this comment

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

how about else @error(...)?

Copy link
Contributor

@argonaut22 argonaut22 left a comment

Choose a reason for hiding this comment

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

mostly best practices suggestions and continued distaste for aircraft_type

may i also recommend a description in the IO part of the docs?

return EI_NOx
end # function EINOx4

function EINOx(ac::aircraft, ip::Int; sp_humidity = 0.00634, method="cubic")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the only place that uses aircraft_type. Would still vote against having it as a field and vote for having it as an input whenever odperf!() is called (which then passes to EINOx and co).

Suggested change
function EINOx(ac::aircraft, ip::Int; sp_humidity = 0.00634, method="cubic")
function EINOx(ac::aircraft, ip::Int, aircraft_type::Sym; sp_humidity = 0.00634, method="cubic")

Copy link
Contributor

Choose a reason for hiding this comment

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

the motivation is that "narrow-body"/"wide-body" is too big of a user-facing lever that does nothing anywhere else. it's unintuitive

Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps a better var name is correlation_type, since that is the extent of its influence

Assumes a default specific humidity of 0.00634 kg water/kg dry air per
ICAO Annex 16 Vol. II (part 2.1.4.1)
"""
function EINOx3(P3_kPa, T3_K, sp_humidity = 0.00634, ac_type = "1")
Copy link
Contributor

Choose a reason for hiding this comment

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

ac_type default is outdated

Also, should this even have a default? the likelihood of error is greater than the convenience

Assumes a default specific humidity of 0.00634 kg water/kg dry air per
ICAO Annex 16 Vol. II (part 2.1.4.1)
"""
function EINOx4(P3_kPa, T3_K, sp_humidity = 0.00634, ac_type = "1")
Copy link
Contributor

Choose a reason for hiding this comment

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

(see above)

println(@sprintf("%12s %12s %12s %12s %12s %12s %12s %12s %12s %12s %12s %12s %12s %12s",
"FL", "TAS", "CAS", "Mach", "Fn", "L/D", "Tt4max", "Tmetmax", "FFmax", "Tt4cruise", "Tmetcruise", "FFcruise", "CL", "CLh"))
# integrate trajectory over climb
for i = 1:N
Copy link
Contributor

Choose a reason for hiding this comment

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

labelled indices are nice

Suggested change
for i = 1:N
for iFL = 1:N

end


## FOR 77W__
Copy link
Contributor

Choose a reason for hiding this comment

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

what in tarnation is 77W

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.

6 participants