Fortran-FOSS-Programmers / WenOOF

WENO interpolation Object Oriented Fortran library
36 stars 17 forks source link

Object Oriented Design: which pattern(s)? #3

Closed szaghi closed 7 years ago

szaghi commented 8 years ago

Hi all,

as I have alreadtingy introduced into issue#2, I would like to exploit Rouson lessons and implement our library via an Object Oriented Way.

Presently, I have not clearly defined which API I would like. Essentially I guess that library should:

In this regards, the input stencils and output interpolated value(s) could be:

The second case is more complex and necessitate an accurate choice of the objects patterns, in particular data encapsulation increases the difficulty and maybe a Factory pattern is not enough. However, also the first more simple API necessity a careful design at least for the internal (to the library) implementation.

Which pattern you suggest and why? I am very newbie in this field. My previous OOP codes are not well designed and do not follow a precise pattern.

szaghi commented 8 years ago

Hi all,

I am going to try the Abstract Factory Pattern as the one shown into the Rouson et. al book Scientific Software Design: The Object-Oriented Way, ISBN:0521888131 9780521888134. Into the book there is a very nice, yet not so simple, application example concerning the solution of the Burgers' equation. You can find the codes here. I am very worried that I have not completely "assimilated" the Rouson's lessons, thus I will try explain my efforts here.

desiderata API

I would like to have an abstract type (the factory type) that will build each actual WENO interpolator under the user demand. Says:

Rouson's Abstract Factory Pattern... in my mind

I woul like to allow the user to perform something like:

  class(weno_interpolator), pointer :: weno
  class(weno_interpolator_factory), allocatable :: weno_creator

  allocate (weno_interpolator_upwind_factory :: weno_creator)
  weno => weno_creator%create(...) ! with the necessary arguments specific for upwind-like weno
  do while (I_want_is_true)
    interpolated = weno%interpolate()
    ...
  enddo   

where:

However, If I understand right (at least superficially) the Rouson's lessons, in order to allow such a scenario, we need at least 4 modules:

  use weno_interpolator_module
  use weno_interpolator_factory_module
  use weno_interpolator_upwind_module
  use weno_interpolator_upwind_factory_module

an abstract WENO interpolator

type, abstract :: weno_interpolator 
  contains
    procedure, deferred :: foo            
    ...
  end type

a concrete WENO interpolator

type, extends(weno_interpolator) :: weno_interpolator_upwind
  contains
    private
    procedure :: interpolate
    procedure :: compute_smothness_indicators            
    ...
  end type

an abstract WENO interpolator factory

type, abstract :: weno_interpolator_factory 
  contains 
    procedure(create_interface), deferred :: create 
  end type 
  abstract interface 
    function create_interface(self, etc...) 
      import :: field_factory
      class(weno_interpolator_factory), intent(in) :: self 
      class(weno_interpolator), pointer :: create_interface
    end function
  end interface

a concrete WENO interpolator factory

type, extends(weno_interpolator_factory) :: weno_interpolator_upwind_factory 
  contains 
    procedure :: create => create_weno_interpolator_upwind
  end type 
contains
  function create_weno_interpolator_upwind(self, etc...)
    use weno_interpolator_upwind_module, only : weno_interpolator_upwind
    class(weno_interpolator_upwind_factory), intent(in) :: self
    class(weno_interpolator), pointer :: create_weno_interpolator_upwind
    create_weno_interpolator_upwind => weno_interpolator_upwind(...)
  end function 

First thoughts

As a consequence: is an abstract factory pattern over-killing the problem? Or simply, did I mislead Rouson et al.?

My opinion is now that a simple abstract type provided as contract to the developers could be enough. Or at least, we can simplify the above pattern, relying on a simpler Factory one (not completely abstract).

A first issue

Different WENO flavors need different inputs and provide different outputs, e.g. the upwind biased approaches (on which I am familiar) typically perform the interpolation on two subsequent locations for avoiding doubling the computations of some variables, whereas other approaches (e.g. CWENO, but I am not sure) could not care about this being better constructed and thus providing the interpolation at just one location. In this case the data hiding and encapsulation poses an issue: how to deal with different interpolators by the same interface? this is the reason why I think to the Abstract Factory Pattern, but the above issues raised up.

zbeekman commented 8 years ago

I’m no expert, on software engineering patterns, but I’m not sure that the abstract factory helps facilitate data reuse as discussed above. (I think the issue is more about the interface than the object creation.) Unless there is a compelling case for using an “abstract factory” pattern over a plain “factory” pattern, I would suggest using the latter. The code can be relatively easily refactored later to transform into the “abstract factory” pattern if need be.

In general one can get a little bit carried away trying to anticipate future needs. I try to adhere to the “don’t build it until you need it” principal.

szaghi commented 8 years ago

@zbeekman I completely agree with you, I am going to play with an even simpler plain factory pattern with some simple approaches for dealing with the different creators. I like your principal do not build it until you need it, thank you.

zbeekman commented 8 years ago

I think the issue about up-winding etc. is basically you need to return two interpolated values for non-symmetric, or upwind biased schemes, and the application can then be in charge of saving the appropriate output for reuse… If some WENO schemes don’t create two outputs then you could just duplicate the single output, and the application would know that it asked for the centered scheme etc.? Does that make sense?

szaghi commented 8 years ago

@zbeekman , sure it has sense. Tomorrow I hope to propose an aproach

szaghi commented 8 years ago

Hi all,

I have just uploaded a first tentative of partially abstract pattern API, release v0.0.2.

I am trying with a non formal pattern.

main abstract types

The API is based on only 2 abstract types that are used like a contract for developers: they (partially) specify a public API forcing developers to respect that API. In some sense they provide also a factory for creating interpolators.

The two types are:

weno_interpolator

type, abstract :: weno_interpolator
  !< WENO interpolator factory object.
  !<
  !< @note Do not implement any real interpolator: provide the interface for the different interpolators implemented.
  !<
  !< @note A concrete implementation must define its own **interpolate** method: it is not provide as abstract deferred type
  !< because it could have very diffirent signature for each different interpolator.
  private
  contains
    procedure(abstract_destructor),  pass(self), deferred :: destroy
    procedure(abstract_constructor), pass(self), deferred :: create
    procedure(abstract_description), pass(self), deferred :: description
endtype weno_interpolator

weno_constructor

type, abstract :: weno_constructor
  !< Abstract type used for create new concrete WENO interpolators.
  !<
  !< @note Every concrete WENO interpolator implementations must define their own constructor type.
  private
endtype weno_constructor

the interfaces of deferred procedures can be seen here

example of concrete interpolator types

For testing this API I have added a tentative of weno_upwind (to be checked!)

weno_interpolator_upwind

type, extends(weno_interpolator) :: weno_interpolator_upwind
  private
  integer           :: S = 0              !< Stencil dimension.
  real              :: eps = 1.E-16       !< Parameter for avoiding divided by zero when computing smoothness indicators.
  real, allocatable :: weights_opt(:,:)   !< Optimal weights                    [1:2,0:S-1].
  real, allocatable :: poly_coef(:,:,:)   !< Polynomials coefficients           [1:2,0:S-1,0:S-1].
  real, allocatable :: smooth_coef(:,:,:) !< Smoothness indicators coefficients [0:S-1,0:S-1,0:S-1].
  contains
    ! public methods
    procedure, public :: destroy
    procedure, public :: create
    procedure, public :: description
    procedure, public :: interpolate
    ! private methods
    final :: finalize
endtype weno_interpolator_upwind

weno_constructor_upwind

type, extends(weno_constructor) :: weno_constructor_upwind
  integer :: S = 0 !< Stencils dimension.
endtype weno_constructor_upwind
interface weno_constructor_upwind
  procedure weno_constructor_upwind_init
endinterface

Rationale

The main reason to have 2 abstract type instead of only is to allow (in near future) to implement interpolators having very different arguments and in the meanwhile to retain a general API as seamless as possible: the interpolators are create and used almost identically, but the constructors may strongly differ.

Example of use

I added also two test programs (to compile use the provided fobos) showing the API usage.

type(weno_constructor_upwind)         :: upwind_constructor       !< WENO constructor.
class(weno_interpolator), allocatable :: weno                     !< WENO interpolator.
...
upwind_constructor = weno_constructor_upwind(S=S)
allocate(weno_interpolator_upwind :: weno) 
call weno%create(constructor=upwind_constructor)

or without polymorphic class

type(weno_constructor_upwind) :: upwind_constructor       !< WENO constructor.
type(weno_interpolator_upwind) :: weno                     !< WENO interpolator.
...
upwind_constructor = weno_constructor_upwind(S=S)
call weno%create(constructor=upwind_constructor)

more details here

First conclusions

I am almost comfortable with this API. What do you think?

P.S. the sin_reconstruction test is aimed to test the upwind WENO for reconstructing the sin function. This is obviously a silly problem, but it is useful for debugging. However, I have written it in really few minutes (even if gnuplot check seems to say ok...) thus very enormous errors are likely present :-)

zbeekman commented 8 years ago

Stefano, it was my understanding that the interfaces were a critical part of the “contract” and I am wondering exactly how you’re going to approach this, if you exclude the interpolate method. If you want client code to have objects that are class(weno_interpolator) without exposing the concrete implementations to the client code, then you will only be able to call methods defined by weno_interpolator. (This is because client code only uses the module with the abstract type and not any of those for the concrete implementation.) As far as I can tell the first (“polymorphic” )example won’t work unless you at least add abstract interfaces to perform the interpolation too.

szaghi commented 8 years ago

Hi Zaak,

Thank you for fast replay.

I start from the end.

As far as I can tell the first (“polymorphic” )example won’t work unless you at least add abstract interfaces to perform the interpolation too.

What do you mean? Indeed, the test provided seems to work well, it compiles with bullet prof checks and produces right (to check yet, tomorrow I will try to add a plot) results, thus it works. On the contrary, if it won't work means that it is an implementation being not formal or standard (in some sense) or smart I completely agree, it is just a test.

it was my understanding that the interfaces were a critical part of the “contract” and I am wondering exactly how you’re going to approach this, if you exclude the interpolate method.

You are right. Presently, I have not provided an interpolate interface because I have not yet figured out the API of a symmetric weno, but as soon as possible also the interpolate method should be a part of the contract.

If you want client code to have objects that are class(weno_interpolator) without exposing the concrete implementations to the client code, then you will only be able to call methods defined by weno_interpolator.

You are right, and if you see the test I must expose the concrete weno interpolator and not just the constructor. Until we will decide which will be the interpolate API we cannot have a concrete Factory object, but we are very close to it...

Can you show me an example of implementation of central (symmetric) weno? The pyweno library is not so clearly implemented for my (very superficial) understanding.

Thank you very much for your help, it is very appreciated!

zbeekman commented 8 years ago

Can you show me an example of implementation of central (symmetric) weno? The pyweno library is not so clearly implemented for my (very superficial) understanding.

I have never used truly centered, symmetric WENO, I have only used Jiang and Shu and “symmetric bandwidth optimized WENO” (SYMBO WENO) from Martin et al. (This is symmetric in the sense that an additional, fully downwind stencil is added, to provide a degree of freedom so that the optimal weights can be optimized for bandwidth resolving efficiency while keeping some small dissipation for wavenumbers that that introduce dispersion errors. The result of this is that it is not a truly central/symetric scheme since some relatively small upwind bias of the optimal stencil is intentionally kept.) I can point you to some PhD theses that describe SYMBO WENO in more detail if you would like a more detailed explanation and implementation details.

As for my points about the interpolate interface: It sounded in your previous post as if perhaps you were not planning on providing an abstract interface for the interpolate method. However adding one will provide some benefits… I’m going to continue this conversation using inline comments from one of your commits to try to be a little bit clearer in illustrating why it will be helpful and what the consequences of your implementation vs one that more closely follows Rouson’s are… time permitting…

zbeekman commented 8 years ago

I added some inline comments to the most recent commit. If I find time I’ll try to work up a PR with some improvements and illustrations of what I mean.

szaghi commented 8 years ago

@zbeekman Thank you very much, you are too kind!

I am sorry I was not clear: I have planned to complete all the interfaces including the interpolate one.

I am going to read your inline comments, thank you again.

szaghi commented 8 years ago

Hi all,

I finally come back to a more standard Factory pattern, as Zaak suggested.

Now the client code can safely deal with only the factory object, the class(weno_interpolator) and the constructors, avoiding to touch each concrete interpolator. This provides an almost seamless interface for strongly different interpolators, as in the case of upwind-biased interpolators with respect the central ones.

The client API should:

As showed into the sin_reconstruction test program the way should be something like

    use wenoof, only : weno_factory, weno_interpolator, weno_constructor_upwind
    type(weno_factory)       :: factory
    class(weno_interpolator) :: interpolator
    ...
    call factory%create(constructor=weno_constructor_upwind(S=3))
    ...
    call interpolator%interpolate(...)

client API

the WenOOF library

It exposes (presently) only:

public :: weno_factory, weno_constructor, weno_interpolator
public :: weno_constructor_upwind

The client entry point is the weno_factory that creates a concrete weno_interpolator subclass object ready to interpolate client data. The creation of the concrete interpolator is done by a concrete constructor, presently only the weno_constructor_upwind (that is the Jiang and Shu one) is available.

the Factory

type :: weno_factory
  !< WENO factory aimed to create and return a concrete WENO interpolator to the client code without exposing the concrete
  !< interpolators classes.
  contains
    procedure, nopass :: create
endtype

the WENO interpolator

type, abstract :: weno_interpolator
  !< WENO interpolator object.
  !<
  !< @note Do not implement any real interpolator: provide the interface for the different interpolators implemented.
  private
  contains
    procedure(abstract_destructor),  pass(self), deferred, public :: destroy
    procedure(abstract_constructor), pass(self), deferred, public :: create
    procedure(abstract_description), pass(self), deferred, public :: description
    procedure(abstract_interpolate), pass(self), deferred, public :: interpolate
endtype weno_interpolator

a note on the weno_constructor

This abstract type is empty: the user should use only a concrete one that each interpolator provides, e.g. the weno_constructor_upwind. Consequently, it will be likely purged out from the public objects of the library. However, for the moment it is public for testing if it is useful for the client-side to allow also the polymorphic behavior of the constructor.

testing status

The sin_reconstruction seems to work even if the result must still be checked carefully.

Concluding remarks (of the day)

I have also added the IR_Precision module in order to make the precision of numbers parametric, namely portable and controllable: feel free to suggest other solutions (other better modules/tricks).

Coming soon

the order is not relevant, I have not planned to strictly follow the list order, but if you prefer other paths let me know

Coming not very soon

cmacmackin commented 8 years ago

I know I haven't been participating in this project and I don't really intend to. However, I'm curious as to why you use your own module for interoperability rather than iso_fortran_env. Admittedly, the latter does have rather long names for things, but you could rename them to be shorter in the use statement.

On 05/08/15 12:27 PM, Stefano Zaghi wrote:

Hi all,

I finally come back to a more standard Factory pattern, as Zaak suggested.

Now the client code can safely deal with only the factory object, the class(weno_interpolator) and the constructors, avoiding to touch each concrete interpolator. This provides an almost seamless interface for strongly different interpolators, as in the case of upwind-biased interpolators with respect the central ones.

The client API should:

  • instantiate a |weno_factory| object;
  • create a concrete |weno_interpolator| object by means of the |weno_factory%create| method through the |weno_constructor_xxx| interface;
  • use only the public methods provided by the class |weno_interpolator|.

As showed into the |sin_reconstruction| test program the way should be something like

 use  wenoof, only : weno_factory, weno_interpolator, weno_constructor_upwind
 type(weno_factory)::  factory
 class(weno_interpolator)::  interpolator
 ...
 call  factory%create(constructor=weno_constructor_upwind(S=3))
 ...
 call  interpolator%interpolate(...)

  client API

    the WenOOF library

It exposes (presently) only:

public :: weno_factory, weno_constructor, weno_interpolator public :: weno_constructor_upwind

The client entry point is the |weno_factory| that creates a concrete |weno_interpolator| subclass object ready to interpolate client data. The creation of the concrete interpolator is done by a concrete constructor, presently only the |weno_constructor_upwind| (that is the Jiang and Shu one) is available.

    the Factory

type:: weno_factory !< WENO factory aimed to create and return a concrete WENO interpolator to the client code without exposing the concrete !< interpolators classes. contains procedure, nopass:: create endtype

    the WENO interpolator

type, abstract:: weno_interpolator !< WENO interpolator object. !< !< @note Do not implement any real interpolator: provide the interface for the different interpolators implemented. private contains procedure(abstract_destructor), pass(self), deferred,public :: destroy procedure(abstract_constructor), pass(self), deferred,public :: create procedure(abstract_description), pass(self), deferred,public :: description procedure(abstract_interpolate), pass(self), deferred,public :: interpolate endtype weno_interpolator

  a note on the |weno_constructor|

This abstract type is empty: the user should use only a concrete one that each interpolator provides, e.g. the |weno_constructor_upwind|. Consequently, it will be likely purged out from the public objects of the library. However, for the moment it is public for testing if it is useful for the client-side to allow also the polymorphic behavior of the constructor.

  testing status

The |sin_reconstruction| seems to work even if the result must still be checked carefully.

Concluding remarks (of the day)

I have also added the IR_Precision https://github.com/szaghi/IR_Precision module in order to make the precision of numbers parametric, namely portable and controllable: feel free to suggest other solutions (other better modules/tricks).

  Coming soon
  • a symmetrical-stencils based interpolator;
  • a WENO-Z interpolator;
  • a 1D Riemann solver test;
  • documentation;

    the order is not relevant, I have not planned to strictly follow the list order, but if you prefer other paths let me know

    Coming not very soon

  • efficiency profiling;
  • scalabilty analysis when coupled into a parallel-computations-framework;

— Reply to this email directly or view it on GitHub https://github.com/Fortran-FOSS-Programmers/WenOOF/issues/3#issuecomment-128038115.

Chris MacMackin Saint Mary's University Curriculum Vitae http://ap.smu.ca/%7Ecmacmack/CV.pdf

szaghi commented 8 years ago

Hi Chris,

I am sorry to read you do not intend to participate... your opinions are always useful.

The iso_fortran_env exposes rather few set of parameters. Into my module there are not only portable kind parameters, but also helpers for performing some common stuff when dealing with numbers (casting to string and viceversa, check the machine endianism, compute a reasonable value of machine zero). Presently, WenOOF could rely on only the iso_fortran_env module, but it can be quite limiting very soon. One problem I am now facing (the object of future issue) is how to construct interpolators that uses huge stencils: we have to deal with long tables of coefficients. In a situations having a number2string and vicevarsa casting facility could greatly help.

However, I am not satisfied with my IR_Precision module. I hope that this is the chance to correct/refactor/modify/destroy/recreate/substitute_with_a_better_other it :-)

See you soon.

cmacmackin commented 8 years ago

Okay, thanks for the info. Once FLATPack is developed hopefully some standard module (possibly IR_PRECISION) will emerge. I wasn't planning to participate in WenOOF because I'm not familiar with WENO interpolation and want to spend my development time on FORD and FLATPack.

On 05/08/15 12:57 PM, Stefano Zaghi wrote:

Hi Chris,

I am sorry to read you do not intend to participate... your opinions are always useful.

The |iso_fortran_env| exposes rather |few| set of parameters. Into my module there are not only portable kind parameters, but also helpers for performing some common stuff when dealing with numbers (casting to string and viceversa, check the machine endianism, compute a reasonable value of machine |zero|). Presently, WenOOF could rely on only the |iso_fortran_env| module, but it can be quite limiting very soon. One problem I am now facing (the object of future issue) is how to construct interpolators that uses huge stencils: we have to deal with long tables of coefficients. In a situations having a number2string and vicevarsa casting facility could greatly help.

However, I am not satisfied with my IR_Precision module. I hope that this is the chance to correct/refactor/modify/destroy/recreate/substitute_with_a_better_other it :-)

See you soon.

— Reply to this email directly or view it on GitHub https://github.com/Fortran-FOSS-Programmers/WenOOF/issues/3#issuecomment-128048949.

Chris MacMackin Saint Mary's University Curriculum Vitae http://ap.smu.ca/%7Ecmacmack/CV.pdf

szaghi commented 8 years ago

@cmacmackin sure, I do not like to waste the time of FORD and FLATPack, they are very important for all of us, go on! Just launch your opinions sometimes when it is possible, as you did here :-)

See you soon.

szaghi commented 7 years ago

Currently, we use a factory pattern in the back-end and we expose only a user-friendly creator driven by some simple user choice. Advanced users who want to implement their own weno-interpolator can always do using the back-end facility (currently non exposed, but it will).