Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add mesh object #95

Merged
merged 40 commits into from
Jun 17, 2024
Merged

Add mesh object #95

merged 40 commits into from
Jun 17, 2024

Conversation

Nanoseb
Copy link
Collaborator

@Nanoseb Nanoseb commented May 17, 2024

This PR implements the ideas discussed in #91

The current progress is the following:

  • mesh module
  • OMP code updated
  • CUDA code updated
  • tests updated

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

src/mesh.f90 Outdated Show resolved Hide resolved
Copy link
Member

@semi-h semi-h left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think mesh class made things a lot better. I left a lot of comments, mostly minor or easy to fix stuff. The most important point is the n variable in tds_solve. The suggested changes in the PR will probably cause some issues with tridiagonal solves, but the fix is easy and doesn't require any big structural changes.

src/mesh.f90 Outdated Show resolved Hide resolved
src/mesh.f90 Outdated Show resolved Hide resolved
src/mesh.f90 Outdated Show resolved Hide resolved
src/omp/backend.f90 Outdated Show resolved Hide resolved
src/omp/backend.f90 Outdated Show resolved Hide resolved
src/solver.f90 Show resolved Hide resolved
src/tdsops.f90 Outdated Show resolved Hide resolved
src/tdsops.f90 Show resolved Hide resolved
src/xcompact.f90 Outdated Show resolved Hide resolved
src/tdsops.f90 Outdated Show resolved Hide resolved
Copy link
Member

@semi-h semi-h left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few more points we might want to discuss in our meeting today

src/mesh.f90 Outdated Show resolved Hide resolved
src/allocator.f90 Outdated Show resolved Hide resolved
src/mesh.f90 Show resolved Hide resolved
@semi-h semi-h mentioned this pull request May 22, 2024
@Nanoseb Nanoseb self-assigned this Jun 6, 2024
@Nanoseb
Copy link
Collaborator Author

Nanoseb commented Jun 12, 2024

Still WIP, but so far the main changes are

  • remove mesh from tdsops
  • add tds_n to tdsops
  • remove n from dirps
  • use tds_n instead of mesh%get_n when working with tds things
  • compute tds_n based on the operation, scheme and from_to (this needs to be implemented)

src/tdsops.f90 Outdated Show resolved Hide resolved
src/tdsops.f90 Outdated Show resolved Hide resolved
src/omp/backend.f90 Outdated Show resolved Hide resolved
@Nanoseb Nanoseb added the core Issue affecting core mechanisms of the software label Jun 13, 2024
src/omp/backend.f90 Outdated Show resolved Hide resolved
src/mesh.f90 Outdated Show resolved Hide resolved
src/mesh.f90 Outdated Show resolved Hide resolved
src/mesh.f90 Outdated Show resolved Hide resolved
@Nanoseb Nanoseb marked this pull request as ready for review June 14, 2024 12:26
@Nanoseb
Copy link
Collaborator Author

Nanoseb commented Jun 14, 2024

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
Copy link
Member

semi-h commented Jun 14, 2024

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
Copy link
Collaborator Author

Nanoseb commented Jun 14, 2024

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

@semi-h
Copy link
Member

semi-h commented Jun 14, 2024

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
Copy link
Member

semi-h commented Jun 14, 2024

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.

src/solver.f90 Outdated Show resolved Hide resolved
@semi-h
Copy link
Member

semi-h commented Jun 17, 2024

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
Copy link
Collaborator Author

Nanoseb commented Jun 17, 2024

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
Copy link
Member

semi-h commented Jun 17, 2024

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
Copy link
Collaborator Author

Nanoseb commented Jun 17, 2024

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.

@Nanoseb Nanoseb merged commit f84015b into xcompact3d:main Jun 17, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Issue affecting core mechanisms of the software
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Naming and storage convention for grid information
2 participants