Refactor the Permittivity function

[Gabrielgerez]

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]

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%.

❗ 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 ⚠️

Additional details and impacted files

@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[robertodr]

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

[Gabrielgerez]

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

[robertodr]

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>?

[Gabrielgerez]

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]

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

[robertodr]

very good 👍🏻

[robertodr]

there is some duplication between base and derived class.

We can add a free function (outside the class) to generate headers:

void print_header(const std::string &header, const std::string &formulation, double in_value, double out_value) {
    mrcpp::print::header(0, header);
    print_utils::text(0, "Formulation", formulation, true);
    print_utils::scalar(0, "Value inside Cavity", in_value, "(in)", 6);
    print_utils::scalar(0, "Value outside Cavity", out_value, "(out)", 6);
}

then in this class:

virtual void printHeader() const { print_header("Step function of Cavity values", "Standard", getValueIn(), getValueOut()); }

while in Permittivity:

void printHeader() const override { print_header("Solvation Cavity", this->formulation, getValueIn(), getValueOut()); }

[Gabrielgerez]

Slightly harder than expected, ended up adding it to the detail namespace