xcompact3d / x3d2

https://xcompact3d.github.io/x3d2
BSD 3-Clause "New" or "Revised" License
3 stars 4 forks source link

'data_loc' in fields #98

Open semi-h opened 3 months ago

semi-h commented 3 months ago

Related to #91 and the discussions in #95. (particularly https://github.com/xcompact3d/x3d2/pull/95#discussion_r1609690019)

This issue is not about the mesh object, or the data_loc variables it defines, these are solid and finally bringing a functionality we very much need in the codebase. This issue is mainly about the data_loc variable in field, and discussing the best way to manage it.

I think managed to come up with a plan where we can have a correct data_loc in most fields (all of them in practice), with minimal user intervention.

My main concern is about making the user responsible for specifiying the data_loc when requesting a field.

dpdx_x => get_block(DIR_X, X_FACE)
call tds_solve(dpdx_x, p, x_stagder_p2v)

I think its not the best way to move forward, because we're taking effectively one action, but in two stages, and unnecessarily link them together making a potential mistake more likely.

So essentially, the tdsops instance x_stagder_p2v in the above code snipped already has got enough idea about where the input field data lives and where the output field data lives. I'll go into more detail but my proposal is basically nominating the tds_solve subroutine to determine the data_loc of the output field based on the input fields data_loc (if available, but will be available almost in all cases) and tdsops instance specifications. And once this is sorted we can rest assured that the output field nows about exactly where the data it store lives at.

dpdx_x => get_block(DIR_X)
call tds_solve(dpdx_x, p, x_stagder_p2v)
! print *, 'data_loc of dpdx_x:', dpdx_x%data_loc

We still need to define the data_loc by ourselves for the main field variables (u, v, and w), but the rest will be sorted out by itself because the tdsops instances can give the correct insight about data_locs.

The only minor issue for implementing this is that tdsops instances are essentially 1D operators, and they can be used in different pairs of data_locs. For example, x_der1st can be used to obtain derivatives from VERT to VERT, as well as from Y_EDGE to Y_EDGE, (also applies to X_FACE-X_FACE and Z_EDGE-Z_EDGE). And to keep things in good order I think the best is coming up with a terminology in tdsops so that we can deduce the output field's data_loc based on input field's data_loc and this newly introduced variable in tdsops, where it describes the relationship between the data_locs of input/output fields.

I think this requires more careful thinking and discussions and of course implementation of this is beyond the scope of #95. I'm looking forward to hear everyones opinion on this and will be happy to give this a try (after #95 is merged) and implement if we're all happy.

Nanoseb commented 3 months ago

I like this idea as it allows to double check that tdsops is doing what we expect it to do and it forces us to set data_loc for inputs fields (which should be more obvious for the user). I will let you implement that in a new PR, but I will add a setter for data_loc there that tsdops can use in the future.

semi-h commented 2 months ago

I just want to elaborate on what I have in mind to make sure we're on the same page.

So the main thing is setting a data_loc in the output field_t before the tds_solve subroutine returns. This could be as simple as

u_out%data_loc = output_data_loc

or with a setter function like

u_out%set_data_loc(output_data_loc)

and probably setter function is a better idea and I think this is what you mentioned. But here this approach simply disregards the present data_loc and just sets it to its new value, and this new value is determined beforehand independently (more details below). The data in output field is also ignored and overwritten so I think it makes sense to just set the data_loc without paying attention to its previous state. Is this also what you have in mind?

The only challenge at this stage is finding the right data_loc for the output field by using input field's data_loc and the tdsops object specifications. There are two ways to sort this out and I think the best way is having an independent function in common.f90, and passing data_loc of the input field and the specific rule in the tdsops class (for now we only have p2v and v2p, but this need to be extended to cover v2v and p2p and maybe with a better naming) so that we get back the data_loc of output field based on these two inputs from this independent function.

Another option would be making tdsops class understand data_locs and figuring out the output data_loc, but I think its not a good idea. tdsops class essentially describes the rules for solving a single tridiagonal system, even applicable to a single RHS, so quite abstract. The pencil group or mesh are all very high level concepts at tdsops class level. data_loc in particular describes a location in a 3D domain, but tdsops is not concerned about higher dimensions and absracted away from all these. The codebase uses tdsops in a 3D domain, but the input and output data locations as far as tdsops concerned are essentially movements in a 1D line. So what this represents in a 3D domain is something we need to figure out outside of the tdsops in my opinion. Any thoughts on this?

Nanoseb commented 2 months ago

yes, the setter is what I had in mind, and yes, I feel like we shouldn't check data_loc when setting it. For the way to find the right data_loc I also agree, a single function (in common or elsewhere) that will return data_loc make sense. I wouldn't have minded having it in tsdops but you have a better understanding/vision of what tsdops should include that I leave that call to you