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

Refactor the solver module to a more general and class-based system #503

Draft
wants to merge 23 commits into
base: develop
Choose a base branch
from

Conversation

RHammond2
Copy link
Collaborator

@RHammond2 RHammond2 commented Sep 20, 2022

Feature or improvement description
This is a refactoring of floris/simulation/solver.py to have less repeated code blocks by moving from function-based operations to class-based operations with a base Solver class to implement the initialization and expected methods. In particular, the full flow solvers all followed the same routine, so this is implemented once in Solver.

Related issue, if one exists

Impacted areas of the software

  • floris/simulation/solver.py
  • floris/floris.py (when new API is settled)

Additional supporting information

This relates to a discussion point where the solver needed to be reconsidered because it was growing too much and having too many of the same code patterns being reused, so moving to a different module-level architecture was desired.

Solver

  • Base class defining all of the common inputs and their validation routines
  • Provides the full_flow_solve() method, which is the same for each child class, so that only solve must be defined for any new solver
  • solve()
    • Provides an abstractmethod to ensure a new solver must define this method to be valid
    • Provides the expected inputs for the model

SequentialSolver
Provides the previous sequential_solver() method as SequentialSolver.solve().

CCSolver
Provides the previous cc_solver() method as CCSolver.solve().

TurbOParkSolver
Provides the previous turbopark_solver() method as TurbOParkSolver.solve().

EmpiricalGaussSolver
coming soon

Remaining Questions

  1. Does this layout meaningfully solve our needs of using so much recycled code throughout the module?
  2. Do farm, flow_field, and grid need to be redefined in XSolver.solve()? This feels redundant for the most part, and is likely a remnant of directly porting over the current solver API usage.
  3. Why does the full flow solver make a new deepcopy of the farm and flow_field objects, and do we need to still do that if we move to it being a class attribute?

Test results, if applicable

@RHammond2 RHammond2 added enhancement An improvement of an existing feature v3.3 labels Sep 20, 2022
@RHammond2 RHammond2 requested a review from rafmudaf September 20, 2022 21:11
@RHammond2 RHammond2 marked this pull request as ready for review July 11, 2023 16:27
@rafmudaf rafmudaf self-assigned this Jul 12, 2023
@rafmudaf rafmudaf added this to the v3.5 milestone Jul 12, 2023
@rafmudaf
Copy link
Collaborator

rafmudaf commented Jul 13, 2023

Here's a diagram of the current solver structure to support the various models for turbine-grid calculations. The visualization routines are in a similar structure. @RHammond2 can you describe how this pull request changes this structure?

classDiagram

    class Farm

    class FlowField {
        array u
        array v
        array w
    }

    class WakeModelManager {
        <<interface>>
    }

    class Solver {
        <<interface>>
    }
    class sequential_solver {
    }
    class turbopark_solver {
    }
    class cc_solver {
    }
    class empirical_gauss_solver {
    }

    JensenVelocityModel --> sequential_solver
    GCHVelocityModel --> sequential_solver
    TurbOParkVelocityModel --> turbopark_solver
    EmpiricalGaussVelocityModel --> empirical_gauss_solver
    CumulativeCurlVelocityModel --> cc_solver

    sequential_solver --> Solver
    turbopark_solver --> Solver
    cc_solver --> Solver
    empirical_gauss_solver --> Solver

    Solver --> Grid
    Solver --> Farm
    Solver --> FlowField
    Solver --> WakeModelManager
Loading

@RHammond2
Copy link
Collaborator Author

RHammond2 commented Jul 13, 2023

@rafmudaf the above structure doesn't meaningfully change, it merely modifies the solver internals to reduce redundant code that can be tricky to consistently maintain.

@rafmudaf rafmudaf modified the milestones: v3.5, v4.0 Aug 2, 2023
@rafmudaf rafmudaf removed this from the v4.0 milestone Nov 29, 2023
@rafmudaf rafmudaf marked this pull request as draft December 6, 2023 20:25
@misi9170
Copy link
Collaborator

After discussion with @RHammond2 and other FLORIS developers, this PR is on hold but the general concept of refactoring the solver code is still of direct interest and is slated for future work. Ideally, the various solvers would be unified, either using a class-based system so that inheritance may be employed, and/or a single solver function created that works with all wake models, to minimize repeated code.

@misi9170 misi9170 added the on-hold work on pause label Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An improvement of an existing feature floris.simulation on-hold work on pause
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants