xcompact3d / x3d2

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

Add mesh object #95

Closed Nanoseb closed 2 months ago

Nanoseb commented 3 months ago

This PR implements the ideas discussed in #91

The current progress is the following:

The code function mesh%get_n isn't finalised yet because I am a bit unsure of the actual result it should return depending on the periodicity of the BC and the data_loc (VERT, CELL, etc.) @pbartholomew08 or @semi-h could you have a look at it?

closes #91

Nanoseb commented 2 months ago

Still WIP, but so far the main changes are

Nanoseb commented 2 months ago

I believe it is now ready to be reviewed/merged. The only thing I couldn't test was the CUDA backend, but if it compiles and the tests pass it should be fine.

semi-h commented 2 months ago

I can checkout and test the PR on CUDA to make sure. Is it okay if I push into this PR some commits to fix if there are any minor issues we didn't catch by eye on the CUDA backend?

I think we still don't have the automatic formatting set up, so can you run fprettify manually if you haven't done so?

Nanoseb commented 2 months ago

yes, go ahead with testing with cuda (and pushing here). And good point about fpretiffy, I will run that

semi-h commented 2 months ago

At the current stage it compiles and tests are passing, however the TGV case is failing on CUDA backend. I'll look into this and let you know when its all sorted.

Working a bit on the new mesh and allocator made me realise that they're a bit tangled to each other, for now it should be alright, but I think we need to look into separating them in a better way maybe. I can open an issue to point out the things we can try later.

semi-h commented 2 months ago

It was a really weird bug...

For some reason, the mesh%get_n functions getting called inside the poisson_fft modules was giving the wrong value. In the base poisson module the values were correct, but in the backend poisson modules it was always wrong, and a random value.

I had to remove the calls to mesh%get_n inside the backend poisson modules, but still use the mesh%get_n when instantiating the base class and the dimensions. I think its safer in this way anyways, but still don't understand why the mesh%get_n calls inside the backend poisson modules were giving the wrong result. I think we need to keep an eye on this, as it may be a problem in some other parts of the code as well.

Any thoughts why there was such a behaviour? First I thought it was due to mesh member not being defined as target, but it wasn't the issue. We access lots of stuff by reference and never had such an issue.

semi-h commented 2 months ago

I would like to implement something quick with get_dims but wanted to ask your opinion first.

We use get_dims only in solver and allocator, and always with specifying a dir and a data_loc. I think the dir argument for get_dims is redundant. It only returns the 3D dimensions, so dir actually doesn't play a role for gathering this info.

The only case where it will be relevant is when we ask for DIR_X/Y/Z and a data_loc, but we never do this currently, do you have any plan for using this combination? I think this combination can be a bit misleading, because it returns (SZ, n, n_groups), where n is the actual domain size, but SZ and n_groups are inherently padded dimensions.

So if you're not planning to use DIR_X/Y/Z and data_loc combination, shall we change get_dims a bit such that it only inputs a data_loc and gives the relevant dimensions based on data_loc only (also can be overloaded with a field input)?

Or if you think theDIR_X/Y/Z and data_loc combination will be useful, how about renaming this to get_field_dims, where we require a dir and data_loc input, and also have get_dims, where we require only a data_loc input?

Nanoseb commented 2 months ago

Yes, indeed. I implemented get_dims before seeing how it would be used, that makes sense. Though we can also add get_field_dims as it may still be useful at some point (and the name is more clear).

semi-h commented 2 months ago

Yes, indeed. I implemented get_dims before seeing how it would be used, that makes sense. Though we can also add get_field_dims as it may still be useful at some point (and the name is more clear).

Okay, renamed the existing get_dims to get_field_dims so that they can be used if there is need.

Then implemented a get_dims that inputs a data_loc and returns the local domain size.

Also implemented a get_global_dims because the FFT based Poisson solver requires the global dimensions. This required adding two new member variables in the mesh class; global_vert_dims and global_cell_dims. Please check the naming convention and the changes if they all make sense.

Nanoseb commented 2 months ago

Looks good to me. A bit of a shame the logic of get_n is implemented twice (the set of select case...). But I don't see an obvious way to make it better without making it overly convoluted. So I'd say it is good to go.