making master support the model in pk_journal #110
Conversation
|
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 |
|
@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. |
|
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. |
|
Also, if you want to see the progress on documentation, go to the documentation page, v:docs. |
|
@1ozturkbe I want to hear @pgkirsch's thoughts on this PR before we make a final determination |
|
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. |
|
@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. |
|
@pgkirsch @1ozturkbe well...the answers are different on this branch. there's def a bug that is not obvious to me |
|
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? |
|
@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. |
|
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 <[email protected]<mailto:[email protected]>> wrote:
@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<#110 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AITKcywQJRnL_-3rH1z75uSf3g3XGDokks5s2hvZgaJpZM4QSfYt>.
|
|
If I have time over the next couple of days I’ll take another look
… On Nov 14, 2017, at 6:35 PM, pgkirsch ***@***.***> 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 ***@***.******@***.***>> wrote:
@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<#110 (comment)>, or mute the thread<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 <#110 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGe0f9g_rCG-PlzQ45MTwYbxNpBmKSQdks5s2iO6gaJpZM4QSfYt>.
|
|
If I have time over the next couple of days I’ll take another look
… On Nov 14, 2017, at 6:35 PM, pgkirsch ***@***.*** ***@***.***>> 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 ***@***.*** ***@***.******@***.*** ***@***.***>>> 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<#110 (comment) <#110 (comment)>>, 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 <#110 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGe0f9g_rCG-PlzQ45MTwYbxNpBmKSQdks5s2iO6gaJpZM4QSfYt>.
|
@pgkirsch @1ozturkbe
Tasks before merging can be done