Skip to content

Combine single_PAO_to_grid and single_PAO_to_grad#251

Merged
davidbowler merged 7 commits intodevelopfrom
tk-reduce-pao-duplication
Sep 15, 2023
Merged

Combine single_PAO_to_grid and single_PAO_to_grad#251
davidbowler merged 7 commits intodevelopfrom
tk-reduce-pao-duplication

Conversation

@tkoskela
Copy link
Contributor

@tkoskela tkoskela commented Aug 29, 2023

Closes #244

I've merged the two identical subroutines into PAO_or_gradPAO_to_grid. The subroutine called in the inner loop is passed in as a procedure argument through an interface. As a side effect I had to always pass direction into both subroutines to make them conform to the same interface, in evaluate_pao it's just a dummy argument that does nothing. I've updated all calls to PAO_or_gradPAO_to_grid, evaluate_pao and pao_elem_derivative_2 and added a bit of explanation in comments.

Very open to a more descriptive names for the subroutine and the interface

Gets rid of OpenMP overheads in single_PAO_to_grad we were seeing earlier

Previously:

image

This PR:

image

@tkoskela tkoskela linked an issue Aug 29, 2023 that may be closed by this pull request
@tkoskela tkoskela changed the title Tk reduce pao duplication Combine single_PAO_to_grid and single_PAO_to_grad Aug 29, 2023
@tkoskela tkoskela marked this pull request as ready for review September 11, 2023 16:04
@tkoskela
Copy link
Contributor Author

I don't see any noticeable performance impact of passing the subroutine as an argument instead of selecting it with an if statement inside the loop nest. I like not having an if statement at the bottom of the loop nest from a code readability point of view.

contains

!!****f* PAO_grid_transform_module/single_PAO_to_grid *
!!****f* PAO_grid_transform_module/single_PAO_to_any *
Copy link
Contributor

Choose a reason for hiding this comment

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

The name here isn't really quite right, but I can't think of an alternative! The previous names (single_PAO_to_grid and single_PAO_to_grad) but also misleading (the second should have been single_PAO_grad_to_grid).

I wonder if single_PAO_to_grid would be better though it might confusing historically

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand the algorithm well enough to give the subroutine a descriptive name, so I'm really waiting for your input here 😅 If you think single_PAO_to_grid is good, let me know.

Copy link
Contributor

Choose a reason for hiding this comment

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

Possibly single_PAO_or_grad_PAO_to_grid which is at least descriptive! I don't think that we need the single part, so maybe PAO_or_grad_PAO_to_grid. @tsuyoshi38 and @ayakon do you have any thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

PAO_or_grad_PAO_to_grid sounds reasonable to me. (I would like to remove _ between grad and PAO, like PAO_or_gradPAO_to_grid

Copy link
Contributor Author

@tkoskela tkoskela Sep 14, 2023

Choose a reason for hiding this comment

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

Technically this should be "any subroutine that satisfies the evaluate interface to grid" but I suppose we can name it after the current implementations. Would transform_to_grid be too generic? When you call the subroutine you'll see what you are transforming from the arguments, for example

call transform_to_grid(tmp_fn, pao_elem_derivative_2, dir1)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that would risk confusion as this routine is specifically for PAOs and their derivatives (there are other functions transformed to the grid). I've taken what @tsuyoshi38 and I agreed (PAO_or_gradPAO_to_grid) and pushed it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. Ready to merge then?

Copy link
Contributor

@ilectra ilectra left a comment

Choose a reason for hiding this comment

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

Only minor aesthetic comments

@tkoskela
Copy link
Contributor Author

Only minor aesthetic comments

Thanks! All done now

Changes wherever routine is called; now more descriptive
subroutine name
@davidbowler davidbowler merged commit 75273fd into develop Sep 15, 2023
@davidbowler davidbowler deleted the tk-reduce-pao-duplication branch September 15, 2023 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reduce code duplication in PAO_grid_transform_module

4 participants