-
Notifications
You must be signed in to change notification settings - Fork 69
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
User Interface for Modeling Aircraft #462
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this, Dawson! I tried it out locally and it worked first try. I like the collapsible headings for each subsystem.
I added some small comments and I'll put some general ones here:
- could you please add a command mapping for this utility so that someone could do
aviary build_model
(or whatever name you choose) in the command line, like the other utilities Aviary has? There's a little bit of convoluted code to do this, but you can follow the other utilities' logic here or I'd be happy to walk through this process with you - when you hit
Submit
could you make some sort of indication that a file was saved? I hit submit and initially thought it didn't work because I didn't see a confirmation or similar message, though a file was successfully created. This notification could be a message in the command-line window, a pop-up in the GUI, or something else. I think it'd be useful to let the user know that a file was made explicitly
Feel free to disregard any of these suggestions, I just wanted to weigh in. Thanks for the nice work!
aviary/interface/build_model.py
Outdated
from tkinter import * | ||
import tkinter as tk | ||
import tkinter.ttk as ttk | ||
from tkinter import messagebox | ||
from tkinter import filedialog |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of these imports are unnecessary; with the from tkinter import *
all parts of tkinter are imported. You could keep either that line (not too common to do) or explicitly import the specific things you need from tkinter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the cleanest is just to have the 2 aliased imports (tk, ttk), and use those to call all needed functions. The two functions specifically imported here don't seem to be used more frequently than other tkinter functions, so to me there isn't a clear reason to specially import those.
I'll weigh in that we shouldn't globally import here. That's something we should only do if there is a very good reason to specifically do that.
aviary/interface/build_model.py
Outdated
else: | ||
entries_per_subhead.append(1) | ||
name_each_subhead.append(key) | ||
headers.append('dynamic') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dynamic variables (all those that are Dynamic.Mission.<variable_name>
are not actually used in the .csv file. Because of this, I don't think they need to be included in this GUI.
There could be value in having the dynamic variables present in the GUI and you then output those values as initial guesses used in the phase_info
object, but I think that capability is already represented by the draw_mission
tool. So, I'd suggest removing the dynamic variables from this GUI unless I'm missing a reason (which I easily could be!)
With new commits, the gui can now be ran using aviary command . Additionally, all Dynamic.Mission.**vars were removed along with the associated tab. The gui will now indicate to the user via command prompt when a file has been saved with "Summit", "File->Save", and "File->Save as...", The gui will now indicate to the user via command prompt when a file has been opened with "File->Open" and "File->Open & Display". |
aviary/interface/cmd_entry_points.py
Outdated
@@ -97,10 +100,11 @@ def aviary_cmd(): | |||
cmdargs = [a for a in sys.argv[1:] if a not in ('-h',)] | |||
|
|||
if len(args) == 1 and len(user_args) == 0: | |||
if args[0] != 'draw_mission' and args[0] != 'run_mission': | |||
if args[0] != 'draw_mission' and args[0] != 'run_mission' and args[0] != 'build_model': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if args[0] != 'draw_mission' and args[0] != 'run_mission' and args[0] != 'build_model': | |
if args[0] not in ('draw_mission', 'run_mission', 'build_model'): |
aviary/variable_info/variables.py
Outdated
@@ -155,7 +155,7 @@ class Design: | |||
FINENESS = 'aircraft:design:fineness' | |||
FIXED_EQUIPMENT_MASS = 'aircraft:design:fixed_equipment_mass' | |||
FIXED_USEFUL_LOAD = 'aircraft:design:fixed_useful_load' | |||
IJEFF = 'ijeff' | |||
IJEFF = 'aircraft:design:ijeff' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is intentional (and does not need to be set by the GUI)
IJEFF = 'aircraft:design:ijeff' | |
IJEFF = 'ijeff' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@crecine
Without this modification "ieff" becomes a subheading and since this variable is in the middle of the "design" section it takes the rest of the design variables into its container. Would it be better to move this variable to later in the variable list or is there a better option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we need this change to prevent that issue, that's fine. But since we don't need to set this value, excluding it from the GUI would also work
There are some variables that have different defaults for GASP vs FLOPS (see utils/legacy_code_data). It might be a good idea to allow the user to load the defaults from one of those before they start making changes. |
|
I meant adding a button (or something else) to allow the user to load one of the legacy defaults after it reads the variable_meta_data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file needs docstring.
Thanks, Dawson! In the search tab, if I put something in the search box, do a search, and then press Clear, the results go away as they should but the search box still has what I typed into it. Should it? I think most search interfaces clear the search text box also. |
When you do a Submit, the text in the terminal has a typo
|
Summary
This commit adds a interface for users to easily create and edit models to use for optimization. Interface includes names, units, and descriptions of each variable and sorts them into four tabs for Aircraft, Dynamic, Mission, Setting and sorts each variable name into containers based on the subheading "wing", "engine", etc. Additionally, there is an added search feature which allows for users to quicky search a variable name to be edited. Once finished with entering inputs, the user's inputs and remaining entries, which autofill to default values, are written to a csv file in a format that can be used by Aviary. Image of interface attached below.
All information is pulled from Aviary>aviary>variable_info>variable_meta_data.py and Aviary>aviary>variable_info>variables.py thus it automatically updates and adds variables to the interface when variables are added to Aviary's meta data.
Additional edits were made to Aviary>aviary>variable_info>variables.py to keep formatting consistent with other variables.
Related Issues
Backwards incompatibilities
None
New Dependencies
None