Allow time step to increase after shrinking. #3765
Conversation
|
Results of testing a28a9f2 using moose_PR_pre_test recipe: Passed on: linux View the results here: https://www.moosebuild.com/view_job/4762 |
|
What is the policy on commented-out code? Shall I remove that old return line? |
|
Please remove the old line. If we want it back, we'll get it out of the repo. |
|
Results of testing a28a9f2 using moose_PR_test recipe: Passed on: linux View the results here: https://www.moosebuild.com/view_job/4763 |
|
Results of testing a28a9f2 using moose_PR_debug_test recipe: Passed on: linux View the results here: https://www.moosebuild.com/view_job/4764 |
|
👍 |
shanestafford
force-pushed
the
constantDtRegrow_3764
branch
from
a28a9f2
to
0f4a6d8
Compare
|
Results of testing 0f4a6d8 using moose_PR_pre_test recipe: Passed on: linux View the results here: https://www.moosebuild.com/view_job/4771 |
|
Results of testing 0f4a6d8 using moose_PR_test recipe: Passed on: linux View the results here: https://www.moosebuild.com/view_job/4772 |
|
Results of testing 0f4a6d8 using moose_PR_debug_test recipe: Passed on: linux View the results here: https://www.moosebuild.com/view_job/4773 |
|
I hate to overdesign stuff... but it seems weird to hardcode a Go ahead and make the default for that parameter |
This addresses issue idaholab#3764.
shanestafford
force-pushed
the
constantDtRegrow_3764
branch
from
0f4a6d8
to
2918632
Compare
|
Results of testing 2918632 using moose_PR_pre_test recipe: Passed on: linux-gnu View the results here: https://www.moosebuild.com/view_job/4788 |
|
Results of testing 2918632 using moose_PR_test recipe: Passed on: linux-clang,linux-gnu View the results here: https://www.moosebuild.com/view_job/4789 |
|
Results of testing 2918632 using moose_PR_debug_test recipe: Passed on: linux-clang,linux-gnu View the results here: https://www.moosebuild.com/view_job/4790 |
|
@permcody the const strings are because those literals show up at least twice (sometimes more) in the .C files. Advantages are:
|
|
Agreed on all counts. However: We'll merge this, but generally we prefer uniformity. We could put a ticket in to change all of MOOSE but in places where we have 20 parameters we'll have this nasty const block listing 20 parameters. |
|
Just FYI, I claim no credit for the idea; it's a common software engineering methodology and is required at some software companies for every magic valued literal -- especially for duplications. I did not think the MOOSE coding standards applied here since it is not a question of style. I agree that style should be uniform, but when we can improve quality, what better time to improve things than while we're in there taking a fresh look at older code? |
|
I understand the pros of this approach - but... in addition to being different from everything else... the proliferation of strings gets ridiculous. I actually did a "MOOSE-like" project at Sandia where I took this approach... and it gets crazy. You end up with header files FULL of this stuff and while it may be easier to "maintain" (in some software engineering sense), the non-locality of the changes makes it annoying. Think about things like Material properties. Let's say you have a property called "thermal_conductivity". Ideally you would like to define that as a const string - then everyone uses that and you get compile-time errors for mispellings (yay?). However, WHERE are you supposed to declare those? No one "owns" "thermal_conductivity". Maybe we should create a "MooseStrings.h" file... that's a ridiculously huge list of strings that people "might" be interested in? I could go on forever about why this looks like a good idea but in reality it just creates non-local dependencies between code that are (at best) annoying and (at worst) another barrier for a new developer (hey - where do I go to declare my strings? What if I depend on this module... does it have it's own place to declare strings?) All in all - I vote no. |
|
Uniformity is about much more than just capitalization and spacing. We also enforce uniformity in the algorithms we use and the general flow of the code. To get an idea of some of the stuff we care about look at the "Commandments" at the bottom of http://mooseframework.org/wiki/CodeStandards/ . There are many ways to do everything in programming... we try to do things uniformly so that it's easy to move through and work with the code. |
|
Cody I understand your concern and respect your prerogative to control the code, so feel free to reject the pull. Derek, your arguments about data non-locality are all obviously correct, and I 100% agree that trying to implement a database of material property strings using C++ headers would be a bad idea. But you'll have to help me understand how these concerns relate to the current code, which is two strings for parameter names that are entirely local to a single translation unit, restricted by the anonymous namespace. |
|
If we were going to predeclare const strings somewhere... we are going to do it everywhere. We only do things uniformly in MOOSE - so a decision to do this here is a decision to do it everywhere. That's why it relates to the current changes. |
|
Just to be clear, your opinion is based on your own extrapolation of this method far beyond the context of these changes. I never suggested anything of the sort. I also wouldn't suggest that just because a pointer makes sense in one place that it now has to be used everywhere so that we can do it uniformly. Horses for courses. |
|
lol - if you want to continue this conversation... come stop by. (BTW - We explicitly have rules about using Pointers vs References so that we do do that uniformly!) |
|
Your definition of uniformity for MOOSE is not really something I want to come over and discuss privately, and I think it will help people on github to know the specific reason why you object to this code. We both agree that my branch has good code and we both agree that your proliferation scenario would be a disaster. So people reading this (including myself) are going to wonder why the uniformity clause applies in this case but not in others. Why not put a specific "coding standard" rule on the wiki that excludes this? Then we can avoid any future discussion that boils down to a difference of opinions. |
|
@shanestafford Honestly - I think we've explained it pretty well. The point of reviews is that a code team gets to look at a set of changes and decide if they are in line with what they want in the codebase and make requests for modifications to those changes if they don't fit. I honestly can't tell if you're joking about all of this or not. I was just bantering back and forth 'cause it's fun to talk through these things... but if you really don't want to change your code then I'll just close this PR and we can all move on... |
|
lol - I didn't mean to click "Close and Comment" on that ;-) I also didn't mean to send that message out as I don't really think that a fight about "global" is worth it ;-) |
|
I have put in a pull request to @shanestafford with fixes and a test that will get this PR in the right state. You can find it here: shanestafford#1 |
|
Hi, I just wanted to comment on the use of const strings and Material properties (and not on Shane's use of const strings because you are all greater software engineers than me!). In my experience this is a super bad idea. I have a finite-element code of ~160k lines where this is used and it is nightmare trying to add stuff. Originally we had the dream that it would be better, but it turned out super bad. Don't let anyone convince you otherwise! a |
|
Thanks @WilkAndy - that's my experience as well. Sometimes software engineering "best practices" don't work when applied in reality 😄 |
|
@shanestafford, Can you merge Derek's PR into yours and push? Let's get this PR merged. Thanks |
|
IterativeAdaptiveDT seems to be the stepper of choice, so it turns out I don't need this change either. Closing due to lack of interest. |
|
@shanestafford lack of interest in what? All you needed to do was merge my PR to you on this branch. Just click one button and then we can merge this useful capability... |
|
Closing in favor of #3855 |
This addresses issue #3764.