Masked fld file

[adperezm]

Hi,

Here I add very simple code that allows to write out portions of the domain in an fld_file.

The way I do now in the user module is:

    call output%write_masked(temp, my_point_zone%mask, time%t)

The problem here is that the output must be of type fld_file_t. I can't initialize it as file_t because that one does not have the write_masked procedure. So for this I ask your help on how you think we should make it as seamless as possible (if you have ideas).

This would work if one uses the "full_element" option for the point zones, so eventually one could have a general option that allows to automatically write out whichever part of the domain is needed.

I duplicated a lot of code on purpose. I felt that I would end up with a lot of extra optional arguments and a lot of IF statements if I did not do that. Let me know if you think I should change that too.

Again, I would have prefered that fld_file_t does not handle this type of stuff. It is just a file and should not care about masks, logic, etc. But the other option of reducing inputs to contiguous memory and all that was much more intrusive.

[adperezm]

Also, this should not interfere much with #2300, I will merge those changes if they are accepted.

[vbaconnet]

I think this is really great!

I also agree with you that copying the existing writer and just making minimal changes is tgood, it makes it easier to understand exactly what changes between the "regular" and "masked" writers.

Regarding how to manage the actual calling of the masked fld writer, I think it's probably fine for now to not have it pass through the file_t and have it a bit hacky. If this becomes a simcomp then one could take care of all of that from the simcomp.

I think your implementation is clean and well-contained, so if for some reason it needs to be passed down the file_t pipeline later we can deal with those gymnastics then, with whatever interfaces etc.

[timofeymukha]

Highjacking to start a thread.

One way to not duplicate things is to have the fld_file_write call this routine with a mask that includes the entire field, right? It will be at the cost of allocating an array of ints of the size of the dofmap, right?

What one could do is have an element-based mask type, that would reduce the necessary allocation size drastically. That mask could sit in a point zone along with the GLL mask, basically indicating which elements are touched by the zone, even if not necessarily filled when full_elements is false.

Just some ideas.

[adperezm]

Yes! One could do that and always have a mask. But I doubt everyone will agree to that haha. I also like that the clean version exist.

Regarding the element mask option. I would generally agree. In the past, in the only CPU code I used to use element-based masks. But I feel that GLL-based masks are more dominant in Neko, and I wanted to keep everything as consistent as possible. That's why in the full_element modification to the point zone code I explicitly decided to do it this way haha.

We can change it tho, I am not married to this either.

[vbaconnet]

One way to not duplicate things is to have the fld_file_write call this routine with a mask that includes the entire field, right? It will be at the cost of allocating an array of ints of the size of the dofmap, right?

Yeah, it would save some lines of code but idk I think it's a bit scary to mix masked operations with non-masked operations.. I think masked things should be kept separate in a way that their usage should be obvious and intentional. I don't have any good argument for not doing it but it just feels wrong hahah

[timfelle]

I think it would be quite clear if the mask was an optional component. If the subroutine is called with the mask, then we mask the output, if it is not there, you write the whole array. That would be very explicit, and essentially be identical to 2 subroutines, from the caller perspective. I do not see the added value to specify that the operation is masked, if the mask is also an input to the subroutine. Had the mask been part of the type, then it is an entirely different matter.

call file%write(data, t)              ! No masking
call file%write(data, t, mask)        ! Masked
call file%write_masked(data, t, mask) ! Kinda telling it twice

[adperezm]

I agree. But I would like to still keep the separate routines, since otherwise there will be a lot of if-else. I could do something like this (adding a helper subroutine):

  !> Manage writer to use
  subroutine fld_file_write_manager(this, data, t, mask)
    class(fld_file_t), intent(inout) :: this
    class(*), target, intent(in) :: data
    real(kind=rp), intent(in), optional :: t
    type(mask_t), intent(in), optional :: mask
    
    if (present(mask)) then
       if (present(t)) then
          call this%write_masked(data, mask, t)
       else
          call this%write_masked(data, mask)
       end if
    else
       if (present(t)) then
          call this%write_all(data, t)
       else
          call this%write_all(data)
       end if
    end if

  end subroutine fld_file_write_manager

However, I would need to modify the parent type, since otherwise I get:

Error: ‘write’ at (1) must have the same number of formal arguments as the overridden procedure

So that seems a bit disruptive. Since this works only for the fld output, maybe it is not so bad to leave it a bit redundant with the way it is, and let a simcomp or so manage it.

(making write_all and write_masked private.)

[timfelle]

Well, no you do not need to check for the pressence of t if that is also optional in the called functions, so that simplifies it quite a lot.

but sure, the fact that the function is defined in the parent is a bit of a hassle here.

[timofeymukha]

So I guess the whole thing above is shared, mask or not? Then the minumum de-duplication would be to brancj it out in a separate subroutine.

[adperezm]

Yes, indeed. That would be a simple solution I did not think about haha.

[adperezm]

I suggest we keep the alternative routine to be used only by cases that use fld files. Then if we add masks to other types of file, then we add better support.