search

adcirc / adcirc

ADCIRC Model Repository
https://adcirc.org
Other
37 stars 16 forks source link

Make -DDEBUG_WARN_ELEV default behavior. #234

Open mattbilskie opened 4 years ago

mattbilskie commented 4 years ago

Generating a fort.69 for an unstable run should be based on NFOVER alone and not also compiling with -DDEBUG_WARN_ELEV.

tgasher commented 4 years ago

I've dug through the code a little, and it looks like all that's needed is to change the default value to WarnElevDump = .True. in read_input.F. @jasonfleming , any reason to not do this?

zcobell commented 4 years ago

We talked about this briefly during the developer's meeting. It makes sense to me to make this the default behavior or through a namelist configuration instead of a hidden option in NFOVER with a flag. For that to happen, I think there are a couple changes that would be required:

  1. Make the fort.69 a fully-fledged output file instead of where it is now that is an inline output dump
  2. Decide on the namelist or other conventions to control the warning/error elevations. I believe @WPringle has partially tackled this already.
WPringle commented 4 years ago

Yes, the warning/error elevations have been implemented as namelist in a previous commit

tgasher commented 4 years ago

@WPringle @zcobell I agree RE making the file format match something more customary, though there's also value to just getting something in. Of the existing options, the main thing that one might want to change is disconnecting the two uses of WarnElevDumpLimit: (1) how many snaps of data to write, (2) how many warnelevs can go by before the model is stopped.

Relatedly, I don't think I've ever seen the model actually stop due to this limit being exceeded, though I must've seen enough warnelevs for it to have happened...can anyone confirm it does happen?

jasonfleming commented 4 years ago

Yes I agree that you shouldn't need to recompile to get ADCIRC to write a fort.69 (or to activate any other non-developer feature). However, I also don't think that a fort.69 (or fort.69.nc) should be generated automatically when spurious oscillations are detected. We wouldn't want to fill someone's disk quota with a fort.69 when all they really wanted was for the code to exit after a specified number of warning messages. I bet William already thought of being able to turn this on/off in his new warnelev namelist though. :-) While we are at it, it would be cool if the operator could activate a "reach back" feature to optionally force ADCIRC to keep a configurable moving window of fort.69 datasets that it can dump out when spurious oscillations are detected, so we can see what led up to it.

zcobell commented 4 years ago

The WarnElevDumpLimit definitely can stop the model, and it has for me more times than I'm happy to count. I usually set this to 50.

@chrismassey had done some investigation and was able to predict in advance of warning and fatal instabilities by a few timesteps by tracking ETAS coming out of the solver instead of an absolute ETA. Saving too many global time steps could impact memory overhead, but if ETAS is a good predictor, it could serve to signal to ADCIRC to begin tracking things, and by that point, you're already headed down a bad path anyway, so model performance is less of a concern and the attention can turn to diagnostics.

tgasher commented 4 years ago

@zcobell Huh, I guess my instabilities are just really violent.

@jasonfleming I would say that your point gives motivation for disconnecting the two usages of WarnElevDumpLimit, but not for changing default behavior of writing a fort.69. A default for a new parameter governing how many to write, WarnElevDumpWriteCount=1, along with adding it to the namelist to permit user control, takes care of that.

Do we want to maintain compatibility with the older practice of compiler flag + values on the NFOVER line? If so, then when the compiler flag is enabled, we'd have WarElevDumpWriteCount be controlled by WarElevDumpLimit.

chrismassey commented 4 years ago

My vote would be to maintain compatibility for a while longer, but let’s get away from the compiler time options for anything except for debugging. I prefer options be controlled by model input files (i.e. the fort.15).

In the time stepping loop, I examined the size of ETAS after the JCG solver call. If max(ETAS)>=2 then a couple of things could be done:

  1. Flag a message that stated at time step #, ETAS>2. Dump into the fort.16

  2. Same as 1 except also list the first few (6 to 10) nodes that were greater than ETAS>2. Dump into the fort.16

  3. Same as 1 and 2, except limit ETAS to be no greater than 2. This was like a smoothing/limiter that alters the solution, which can get the model over a rough period and control the instability. This needs more investigation. If you think about the fact that ETAS is the change in elevation from one time step to the next for each node, a 2 meter elevation change in a single time step is a really large value.

Bests, Chris +------------------------------------------+ Chris Massey, PhD Research Mathematician US Army Eng. Research & Development Center Coastal & Hydraulics Laboratory CEERD-HFC-I 3909 Halls Ferry Rd. Vicksburg, MS 39180-6199

601-634-2406 (office) Chris.Massey@usace.army.mil +------------------------------------------+

From: T notifications@github.com Sent: Thursday, May 14, 2020 1:45 PM To: adcirc/adcirc-cg adcirc-cg@noreply.github.com Cc: Massey, Thomas C (Chris) CIV USARMY CEERD-CHL (USA) Chris.Massey@usace.army.mil; Mention mention@noreply.github.com Subject: [Non-DoD Source] Re: [adcirc/adcirc-cg] Make -DDEBUG_WARN_ELEV default behavior. (#234)

@zcobellBlockedBlockedhttps://github.com/zcobellBlocked Huh, I guess my instabilities are just really violent.

@jasonflemingBlockedBlockedhttps://github.com/jasonflemingBlocked I would say that your point gives motivation for disconnecting the two usages of WarnElevDumpLimit, but not for changing default behavior of writing a fort.69. A default for a new parameter governing how many to write, WarnElevDumpWriteCount=1, along with adding it to the namelist to permit user control, takes care of that.

Do we want to maintain compatibility with the older practice of compiler flag + values on the NFOVER line? If so, then when the compiler flag is enabled, we'd have WarElevDumpWriteCount be controlled by WarElevDumpLimit.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubBlockedBlockedhttps://github.com/adcirc/adcirc-cg/issues/234#issuecomment-628819889Blocked, or unsubscribeBlockedBlockedhttps://github.com/notifications/unsubscribe-auth/ABNKUW2LENX3POZFZP3ZBYDRRQ3ZNANCNFSM4M6DJQZABlocked.

tgasher commented 4 years ago

@chrismassey Yeah, ETAS=2 is huge. Did you run into false positives with something smaller than this? I'd have thought 0.2 m/timestep would be plenty. Also, would it be better to normalize this by the time step (i.e. 2 m/s or whatever the timestep was) in order to generalize?

krober10nd commented 4 years ago

Besides ETAS which is a good idea, another way to detect instabilities is to use the current momentum solution with the triangle edge lengths. With this information, you can check during the simulation if the CFL condition is violated. In general, if this condition is violated, the solution will be ridden with numerical artifacts and it would be good to know ASAP.

jasonfleming commented 4 years ago

very cool idea Keith ... seems quasi Lagrangian?

On Sat, May 16, 2020 at 7:25 PM keith roberts notifications@github.com wrote:

Besides ETAS which is a good idea, another way to detect instabilities is to use the current momentum solution withe triangle edge lengths. With this information, you can check during the simulation if the CFL condition is violated. In general, if this condition is violated, the solution will be ridden with numerical artifacts and it would be good to know ASAP.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/adcirc/adcirc-cg/issues/234#issuecomment-629718900, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABNIEQWOA4CL5BC4HIPB5OLRR4OF5ANCNFSM4M6DJQZA .

-- Dr. Jason G. Fleming Chief Engineer, Seahorse Coastal Consulting 3103 Mandy Ln Morehead City, NC 28557 Tel: (252) 726-6323 Mobile: (252) 269-0962 Web: http://www.seahorsecoastal.com

krober10nd commented 4 years ago

@jasonfleming Yea, you could think of it like that! You're assuming a particle located at one vertex cannot pass by another vertex connected in the mesh connectivity given the predicted velocity.

min connected edge length > velocity*deltaT
chrismassey commented 4 years ago

Great idea Keith. I think that would be a great item to have an option to check during the simulation.

From: keith roberts notifications@github.com<mailto:notifications@github.com> Date: Saturday, May 16, 2020, 6:25 PM To: adcirc/adcirc-cg adcirc-cg@noreply.github.com<mailto:adcirc-cg@noreply.github.com> Cc: Massey, Thomas C (Chris) CIV USARMY CEERD-CHL (USA) Chris.Massey@usace.army.mil<mailto:Chris.Massey@usace.army.mil>, Mention mention@noreply.github.com<mailto:mention@noreply.github.com> Subject: [Non-DoD Source] Re: [adcirc/adcirc-cg] Make -DDEBUG_WARN_ELEV default behavior. (#234)

Besides ETAS which is a good idea, another way to detect instabilities is to use the current momentum solution withe triangle edge lengths. With this information, you can check during the simulation if the CFL condition is violated. In general, if this condition is violated, the solution will be ridden with numerical artifacts and it would be good to know ASAP.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.