search

convexengineering / SPaircraft

Models for commercial aircraft design
http://spaircraft.readthedocs.org
25 stars 17 forks source link

making master support the model in pk_journal #110

Open mayork opened 7 years ago

mayork commented 7 years ago

@pgkirsch @1ozturkbe

Tasks before merging can be done

mayork commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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

mayork commented 7 years ago

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

pgkirsch commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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

pgkirsch commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

Yeah that sounds like an absolute nightmare to debug. If it’s not possible then it’s not possible.

On Nov 14, 2017, at 15:02, mayork notifications@github.com<mailto:notifications@github.com> wrote:

@pgkirschhttps://github.com/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.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/convexengineering/SPaircraft/pull/110#issuecomment-344429278, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AITKcywQJRnL_-3rH1z75uSf3g3XGDokks5s2hvZgaJpZM4QSfYt.

mayork commented 7 years ago

If I have time over the next couple of days I’ll take another look

On Nov 14, 2017, at 6:35 PM, pgkirsch notifications@github.com wrote:

Yeah that sounds like an absolute nightmare to debug. If it’s not possible then it’s not possible.

On Nov 14, 2017, at 15:02, mayork notifications@github.com<mailto:notifications@github.com> wrote:

@pgkirschhttps://github.com/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.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/convexengineering/SPaircraft/pull/110#issuecomment-344429278, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AITKcywQJRnL_-3rH1z75uSf3g3XGDokks5s2hvZgaJpZM4QSfYt. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/convexengineering/SPaircraft/pull/110#issuecomment-344436285, or mute the thread https://github.com/notifications/unsubscribe-auth/AGe0f9g_rCG-PlzQ45MTwYbxNpBmKSQdks5s2iO6gaJpZM4QSfYt.

mayork commented 7 years ago

If I have time over the next couple of days I’ll take another look

On Nov 14, 2017, at 6:35 PM, pgkirsch <notifications@github.com mailto:notifications@github.com> wrote:

Yeah that sounds like an absolute nightmare to debug. If it’s not possible then it’s not possible.

On Nov 14, 2017, at 15:02, mayork <notifications@github.com mailto:notifications@github.com<mailto:notifications@github.com mailto:notifications@github.com>> wrote:

@pgkirsch<https://github.com/pgkirsch https://github.com/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.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<https://github.com/convexengineering/SPaircraft/pull/110#issuecomment-344429278 https://github.com/convexengineering/SPaircraft/pull/110#issuecomment-344429278>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AITKcywQJRnL_-3rH1z75uSf3g3XGDokks5s2hvZgaJpZM4QSfYt https://github.com/notifications/unsubscribe-auth/AITKcywQJRnL_-3rH1z75uSf3g3XGDokks5s2hvZgaJpZM4QSfYt>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/convexengineering/SPaircraft/pull/110#issuecomment-344436285, or mute the thread https://github.com/notifications/unsubscribe-auth/AGe0f9g_rCG-PlzQ45MTwYbxNpBmKSQdks5s2iO6gaJpZM4QSfYt.