OrderN / CONQUEST-release

Full public release of large scale and linear scaling DFT code CONQUEST
http://www.order-n.org/
MIT License
96 stars 25 forks source link

Reduce code duplication in PAO_grid_transform_module #244

Open tkoskela opened 9 months ago

tkoskela commented 9 months ago

The subroutines

https://github.com/OrderN/CONQUEST-release/blob/1378f0359b798362b84177bc4288e97ceff17824/src/PAO_grid_transform_module.f90#L98

and

https://github.com/OrderN/CONQUEST-release/blob/1378f0359b798362b84177bc4288e97ceff17824/src/PAO_grid_transform_module.f90#L270

Duplicate almost all of the code with only minor differences. It would be good for code stability to move the common code into a utility function, or perhaps merge the subroutines into one. Worth keeping in mind if they need refactoring for #198

davidbowler commented 9 months ago

This is a good point. We could easily merge the two and add a flag to select PAO value or gradient to choose whether to call evaluate_pao or pao_elem_derivative_2 (though this latter name should be rationalised for clarity!)

tkoskela commented 8 months ago

The simplest way is to put an if clause in the innermost loop. I've got a draft version of this in tk-reduce-pao-duplication. I'm a bit concerned about the performance implication of an if statement in the innermost loop. I would like to spend a bit of time to explore different options.

tkoskela commented 8 months ago

Procedure pointers in Fortran 2003 could be useful here. According to a comment in this thread the performance gains can be dubious, but worth checking in my opinion. If there is no real performance gain, it becomes more of a question of which coding style we prefer. I don't have a strong opinion on which style is better, another if-layer in the loop nest or an obscure function interface.

Another reference in Intel docs

davidbowler commented 8 months ago

I wonder if it's not easier to just use an abstract interface and pass the required subroutine as an argument, as described in this thread

tkoskela commented 8 months ago

I wonder if it's not easier to just use an abstract interface and pass the required subroutine as an argument, as described in this thread

Yes, that does look easier! Thanks, I wasn't aware you could pass subroutines without pointers