UNST_9562: Extend the data structure for source/sinks with bubble screens #536
Conversation
FlorisBuwaldaDeltares
left a comment
FlorisBuwaldaDeltares
left a comment
There was a problem hiding this comment.
I think it's a great start and it's very nice it already produces output, but I think the new code could use a bit of polish
...l/dflowfm/packages/dflowfm_kernel/src/dflowfm_kernel/timespace/fm_external_forcings_init.f90
Show resolved
Hide resolved
| tmsx = xk(tmcell%nod(1)) | ||
| tmsy = yk(tmcell%nod(1)) |
There was a problem hiding this comment.
tmcell%nod(1) is the x coordinate of the first vertex of the cell polygon, basically you're placing your source-sink on the cell corner instead of its center. I would suggest xzw and yzw instead:
There was a problem hiding this comment.
Oh i was searching for these, thanks.
| do i = kstart, kstart + kmx - 1 | ||
| if (zws(i) >= tmsz) then | ||
| write(srcid, '(A,I0)') trim(id), bubble_source_count + 1 | ||
| call addsorsin(srcid, [tmsx], [tmsy], [zws(i)], [zws(i)], 0.0_dp, ierr) |
There was a problem hiding this comment.
this is super ugly, as you're trying to get the tmsk and tmsy of the netcell just so that addsorsin can call pinpok to re-find the cell number you already know. I'd suggest a new routine called addsorsin_by_cell_number maybe?
There was a problem hiding this comment.
While computationally I agree this is "ugly" (aka inefficient), doing what you suggest will create far more code. I would do this later I guess when we do a bigger test if the impact of this part of code is noticeable. The impact of this is probably LogN or so
There was a problem hiding this comment.
i'm not worried about the impact, it will be small. I just think it is very ugly so i'd like it improved. I discussed a bit and for me it is also ok to fix it later after the POC is "complete", but at some point i'd want it to be fixed. The addsorsin could use some refactoring in any case
src/engines_gpl/dflowfm/packages/dflowfm_kernel/src/dflowfm_data/fm_external_forcings_data.f90
Show resolved
Hide resolved
| character(len=255), dimension(:), allocatable :: fnamwbnd !< polyline filenames associated with wave-energy boundary | ||
|
|
||
| integer :: numsrc !< nr of point sources/sinks | ||
| type BubblescreenData |
There was a problem hiding this comment.
I would expect the BubblescreenData object to contain all relevant information, like a list of source sink objects, maybe the original bubblescreen polyline that was read? the bubblescreen name/id? are you planning to extend this type later?
There was a problem hiding this comment.
I've added total number of source/sinks and the id. The rest let's see after we do the algo and eventually after figuring out how to integrate the ecModule
There was a problem hiding this comment.
Could you also change the type name to t_BubblescreenData? It is convention to use the 't_' prefix for derived types (similar to using the 'm_' prefix for modules)
thomas-pijls
left a comment
thomas-pijls
left a comment
There was a problem hiding this comment.
Great work! I saw quite some style errors, could you fix them?
...l/dflowfm/packages/dflowfm_kernel/src/dflowfm_kernel/timespace/fm_external_forcings_init.f90
Show resolved
Hide resolved
| character(len=:), allocatable :: location_file !< Bubblescreen location file | ||
| character(len=:), allocatable :: discharge_input !< Bubblescreen discharge input file | ||
|
|
||
| integer, allocatable :: crossed_cells(:) !> Indices of crossed cells in network_data::netcells |
There was a problem hiding this comment.
For variable docstrings we use !< instead of !>
| character(len=255), dimension(:), allocatable :: fnamwbnd !< polyline filenames associated with wave-energy boundary | ||
|
|
||
| integer :: numsrc !< nr of point sources/sinks | ||
| type BubblescreenData |
There was a problem hiding this comment.
Could you also change the type name to t_BubblescreenData? It is convention to use the 't_' prefix for derived types (similar to using the 'm_' prefix for modules)
| integer :: num_source_sinks !< nr of point sources/sinks in bubble screen | ||
| end type BubblescreenData | ||
| ! TODO: add support for multiple bubble screens | ||
| type (BubblescreenData), allocatable :: bubblescreens(:) !< bubble screen data |
There was a problem hiding this comment.
Could you put the dimension specification before the ::? i.e. ..., dimension(:) :: bubblesceens
This is specified in the coding guidelines (sec 4.12)
|
|
||
| call find_cells_crossed_by_polyline(xpl_tmp, ypl_tmp, crossed_cells, error) | ||
|
|
||
| if (.NOT. ALLOCATED(error)) then |
There was a problem hiding this comment.
Can you make this lowercase?
| bubblescreen%id = id | ||
| bubblescreen%start_index = numsrc + 1 | ||
|
|
||
| do cidx = 1, SIZE(crossed_cells) |
What was done
addsourcesinkfor all new bubble screen source/sinkEvidence of the work done
Tests
<add testcase numbers if applicable, Issue number>
Documentation
<add description of changes if applicable, Issue number>
Issue link
https://issuetracker.deltares.nl/browse/UNST-9562