Skip to content

making master support the model in pk_journal#110

Open
mayork wants to merge 7 commits into
masterfrom
integrate_pk_journal_with_master
Open

making master support the model in pk_journal#110
mayork wants to merge 7 commits into
masterfrom
integrate_pk_journal_with_master

Conversation

@mayork

@mayork mayork commented Nov 5, 2017

Copy link
Copy Markdown
Contributor

@pgkirsch @1ozturkbe

Tasks before merging can be done

  • debug running detailed flight profile and detailed engine
  • debug running detailed engine and simple flight profile (I'm guessing this has to do with climb constraining something in the engine so it's then unconstrained without climb) (UPDATE: I think this could be due to statelinking not linking the right variables)
  • change subs files and run functions for all aircraft (except optimal 737 which I'm doing as a test case)
  • check cg model for consistence with pkjournal
  • check results from this branch against pkjournal results and current master
  • update tests to run a PKjournal case

@mayork mayork mentioned this pull request Nov 5, 2017
@mayork

mayork commented Nov 5, 2017

Copy link
Copy Markdown
Contributor Author

noting here that I think we need to check the cg model in this branch...I think it's slightly diff from what's in pk_journal due to some lumping

@mayork

mayork commented Nov 5, 2017

Copy link
Copy Markdown
Contributor Author

@1ozturkbe check out my changes here. Do you think you can handle the rest of the tasks in the PR description? I think I got the hard part done.

@pgkirsch

@1ozturkbe

Copy link
Copy Markdown
Contributor

ugh I'm going to be honest with you, I hate this PR. Can't we just have them look at the old commit hash? I guarantee you, as the code keeps evolving, the runs on the pkjournal version will start failing, and then we are going to have to everything back up again to figure out what is going wrong. Also, this is a big waste of time that could be spent doing documentation, which, once complete, would render this PR useless anyways... Lmk if you think I am wrong.

@1ozturkbe

Copy link
Copy Markdown
Contributor

Also, if you want to see the progress on documentation, go to the documentation page, v:docs.

@mayork

mayork commented Nov 12, 2017

Copy link
Copy Markdown
Contributor Author

@1ozturkbe I want to hear @pgkirsch's thoughts on this PR before we make a final determination

@pgkirsch

pgkirsch commented Nov 13, 2017

Copy link
Copy Markdown

Sorry @mayork not sure how I didn't notice this earlier. Thanks for putting it together, I think it's something @whoburg would be happy to see. I also think this would make it easier to do things like directly compare results of having a detailed engine model vs. not having a detailed engine model, if that's something you think will be useful.

I wish the SP aircraft code was generally cleaner and more readable to the uninitiated reader, but that's a battle for a different day. My biggest concern is making sure that this PR doesn't inadvertently introduce a change to the model (do you have a robust way of making sure it doesn't?, e.g. an automated solution diff tool). If you don't think it's a tonne of work then I would support completing this PR, but that's easy for me to say seeing as I'm not the one doing it.

@mayork

mayork commented Nov 13, 2017

Copy link
Copy Markdown
Contributor Author

@pgkirsch I agree with you on the readability. On the bright side, I think we're far ahead of other research codes that are much more impenetrable and often times less stable/extensible to a multitude of models.

My method of result checking is just compare some key system levels values. I think that should be sufficient.

If you are interested in me finishing this branch off and getting it ready to merge I would recommend also zipping all the files in the pk_journal branch and we can add that to the repo with a note that was the code used in your paper.

Thoughts? Should I finish it off? I think I can get it done within the week so we can include a link the proof revision.

@mayork

mayork commented Nov 14, 2017

Copy link
Copy Markdown
Contributor Author

@pgkirsch @1ozturkbe well...the answers are different on this branch. there's def a bug that is not obvious to me

@pgkirsch

Copy link
Copy Markdown

Sad.. I'm guessing that diff-ing the two sets of code won't be helpful to identify discrepancies because there are lots of changes?

@mayork

mayork commented Nov 14, 2017

Copy link
Copy Markdown
Contributor Author

@pgkirsch the issue seems to be that I have either improperly some constraints amongst the detailed and non-detailed models or am messing up the substitutions (different for detailed and non-detailed models)...I spent a while last night debugging and didn't get anywhere. If I have time I'll look again but I think the task is pretty prone to subtle errors due to the sheer number of constraints.

@pgkirsch

pgkirsch commented Nov 14, 2017 via email

Copy link
Copy Markdown

@mayork

mayork commented Nov 14, 2017 via email

Copy link
Copy Markdown
Contributor Author

@mayork

mayork commented Nov 14, 2017 via email

Copy link
Copy Markdown
Contributor Author

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