search

MRChemSoft / mrchem

MultiResolution Chemistry
GNU Lesser General Public License v3.0
27 stars 21 forks source link

Refactor the Permittivity function #465

Closed Gabrielgerez closed 9 months ago

Gabrielgerez commented 11 months ago

Create a Class "StepFunction" which sets up the base structure of any Permittivity-like function. This is in preparation to the pb-solvation and other work, so we can avoid repeating code

codecov[bot] commented 11 months ago

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (2dcd68d) 69.25% compared to head (bea133e) 69.23%.

:exclamation: Current head bea133e differs from pull request most recent head c591de0. Consider uploading reports for the commit c591de0 to get more accurate results

Files Patch % Lines
src/environment/StepFunction.cpp 91.52% 5 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #465 +/- ## ========================================== - Coverage 69.25% 69.23% -0.03% ========================================== Files 179 180 +1 Lines 14975 14980 +5 ========================================== Hits 10371 10371 - Misses 4604 4609 +5 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Gabrielgerez commented 11 months ago

Before I get into actually reviewing, I believe what you've implemented is a step function

ah, yes that is the name. I'll rename it properly

Gabrielgerez commented 9 months ago

Potentially we can have many step functions in a calculation. The way it's written now, each will make their own copy of the Cavity function, which can possibly become a memory bottleneck. Maybe instead of storing a Cavity the classes should store a std::shared_ptr<Cavity>?

This is a good idea, but the DHScreening function might potentially need a different cavity than the one for the Permittivity (Different radii due to Ion accessible surface and such)

Gabrielgerez commented 9 months ago

Potentially we can have many step functions in a calculation. The way it's written now, each will make their own copy of the Cavity function, which can possibly become a memory bottleneck. Maybe instead of storing a Cavity the classes should store a std::shared_ptr<Cavity>?

This is a good idea, but the DHScreening function might potentially need a different cavity than the one for the Permittivity (Different radii due to Ion accessible surface and such)

This was really a fix waiting to happen (cavity was passed as a shared_ptr from molecule in driver.cpp), we can let whoever (us) wants to do anything in the future deal with cavity ownership