Conversation
keesnederhoff
changed the title
- Bump version
- determination required timestep should be based over the flux uv point instead of cell centre nm - renaming variables - doing calculation of average required timestep per cell in finalize - naming to _analysis consistently - make a initialize_timestep_analysis subroutine - keep compute_cell_min_timestep simple in sfincs_momentum instead - fix openacc data management Todo still: - clear log message of most limiting cell - netcdf output working based on combined uv point output to nm cell
…ge_limiting_timestep' - Note that percentage limiting timestep might not be visible when plotting as continuous/patches, if simulation is always limitied by 1 flux link (2 cells)
…tares/SFINCS into 267-timestep-analysis-output
- Also double check still indices for quadtree model output
Leynse
left a comment
Leynse
left a comment
There was a problem hiding this comment.
Great work!
- THough a number of things needed to be changed - minimum required timestep should be determined over flux links, not grid cells
- See also proposed changes to variable names, output limiting as percentage, add message in sfincs.log file etc.
- Tested for a regular and quadtree model
- Compiles for windows and Docker CPU (GPU ongoing)
@keesnederhoff - Can you check whether you agree?
@maartenvanormondt - CAn you check the 4 flux link indexing in _ncoutput.f90, and whether (ndm>0) statements etc are needed?
|
Gents, nice work! I indeed had to add a bunch of neighbor checks for quadtree but I think it is now working as expected (not yet tested for regular grids, but I do not expect issues). I also moved some of the routines around. The heavy lifting to determine required time steps per cell and percentage limiting per cell (these had been determined at uv points, but in the output we want them per cell) is now done in timestep_analysis_finalize (it thought that was a bit neater than doing it in sfincs_ncoutput.nc). The writing to the log file is now done in timestep_analysis_write_log. Can you guys test? |
…ring msk=1 cell, then set to 0 - Windows compiler already stepped over this issue, but gfortran does not
…mestep-analysis-output
…tares/SFINCS into 267-timestep-analysis-output
|
Thanks both for your detailed review and improvements. The code is easy to understand, looks elegant, and also worked great on the examples I had around. I did update the documentation to ensure this matches the implementation in the source code. I think it is ready to be merged! |
…tares/SFINCS into 267-timestep-analysis-output
…tares/SFINCS into 267-timestep-analysis-output
Leynse
left a comment
Leynse
left a comment
There was a problem hiding this comment.
- Great addition, now finally working correctly for GPU as well. Time to merge!
…troduced in PR #280 - Update descriptions
4 participants
No description provided.