Tentative solution to replace FrozenClass

[tlunet]

To follow up on #240 : here is one way to put class parameters as argument, and still allow to display all the main parameters from the controller class.

It is based on a RegisterParams class, currently implemented in Problem.py, but that could serve as (one of the) parent class of each core classes. I've applied this to the advectionNd class, but there is still a few issues to solve ... :

1. the call of super constructor should be done at the beginning of the children constructor. But yet it's done after the settings of nvars, since it needs nvars as tuple. Is the format for this init attribute of the problem class a requirement for all children class, or it is like it just for advectionNd @pancetta ?
2. the ndim argument can always be determined from the nvars one, and should be since there was no "prolongation" of nvars to larger dimension (for instance, if nvars=4 and ndim=3, then nvars is automatically transformed to [4,4,4]). But the current implementation actually does not allows different size for nvars since the the A matrix is generated taking only nvars[0] into account. So should we keep nvars multidimensional for ndim>1 or can we just use an integer for it and set the number of dimension with ndim ? This could also simplify the generation of xv, and only keep xvalues as main attribute, from which the rest can be derived (dx, xv, etc ...).
3. continuing the previous topic, xv can be stored as sparse, which will save memory (especially in 3D) and if we indeed keep the same mesh on each dimension, we could simply store xvalues, which will enough to generate the analytical solution in u_exact.

Other commits may come on this PR (for instance for documentation, etc ...) but let me know what you think about it yet @brownbaerchen

[brownbaerchen]

I like the changes. I am fine with the register params thing. I guess you are right that there are many things that could use a cleanup...

[tlunet]

@brownbaerchen tell me if the implementation changes suit you. Then I'll start to work on the class documentation

[pancetta]

To (finally) answer your question concerning the init format: this is heaviy problem-dependent. Whatever you put as init into the parent's super constructor will be available as self.init. Think of some crazy data you may need to instantiate new data objects (e.g. from FEniCS). This is what init is meant for: this should contain all information necessary to instantiate new data objects and the format is matched by the corresponding data type (mesh in this case).

[tlunet]

To (finally) answer your question concerning the init format: this is heaviy problem-dependent. Whatever you put as init into the parent's super constructor will be available as self.init. Think of some crazy data you may need to instantiate new data objects (e.g. from FEniCS). This is what init is meant for: this should contain all information necessary to instantiate new data objects and the format is matched by the corresponding data type (mesh in this case).

Thanks ! I may have some small improvement to propose for that. On the same topic, why do we need different data-types for u and f ?

[pancetta]

I like improvements, bring it on!

Concerning data types: the IMEX sweeper expects a different type of f than other sweepers. This f needs an implicit and an explicit part .I really don't like splitting the RHS by index (e.g. f[:,0] is implicit, f[:,1] is explicit), so it has to be by data type configuration. Then, one way to make a problem with IMEX structure is to inherit from another problem and override feval and solve, but keeping the rest (more or less). If then the data type of f and u are the same, you need to change more.

Then think of particles, where the u data type is a particle and the f acceleration or velocity or energy or whatnot.

[tlunet]

Ok, I think I finshed the implementation aspect of this PR. The main change is how parameters are given and stored in the base Problem class, with an example of inheritance using the advection_ND class. Children constructor don't need to pass children parameters to parent constructor, but simply store it as attributes at the end, and registers the selected attributes as class parameters. Some may be changed during run-time (for instance, if it will be taken into account for in any other class method that uses it), some may not and can be registered as read-only parameters, which raises an error when one want to modify them. All those parameters will be accessible through the params property, that returns a dictionary containing names and values of each main class parameters.

Also, the problem class has now two properties that can be used to instantiate data types for u and f. It simplifies a little bit the implementation in inherited Problem classes, but the main advantage is to tell explicitly in the code how the init argument must interact with dtype_u and dtype_f (and will be underlined by the documentation).

🔔 Note on testing : in a near future, each inherited problem class could be tested like this ...

def testProblemImplementation(probClass):
    prob = probClass()
    params = prob.params  # check if all registered parameters where set as attributes
    u_init = prob.u_init  # check if dtype_u works with init
    u_init = prob.f_init  # check if dtype_f works with init

If you are OK with this @pancetta @brownbaerchen, I'll write down the full documentation for those parts ...

[brownbaerchen]

Looks good to me apart from minor things. Also, we should probably have either the helpers/pysdc_helper.py file or the core/Common.py file, but not both, since they have the same purpose. I don't care what name the file has, though.

[brownbaerchen]

I personally would like the _register function to accept a dictionary instead of *names. Then don't need the separate steps of storing and registering. But I get the feeling you are not a fan of dictionaries. So if you prefer this, I am fine with it as well.

[tlunet]

It's not that I'm not a fan of dictionary. Actually, one could even store automatically parameters as arguments without dictionaries, like this

self._register('nvars', 'ndim', 'c', 'stencil_type', 'order', 'bc', readOnly=True, localVars=locals())
# => get the parameters values from locals() and store it directly as attribute in _register method

But I'm following one of Python's Zen : "Explicit is better than implicit"

By setting all parameters as attributes in the _register method, you hide the storing of parameters as object attribute, and never write it explicitly, e.g self.nvars = nvars. From a developer point of view, once you are aware of this mechanism, it is indeed simpler to write and avoid this whole self.blabla = blabla multiple times.
But this is only for one person, and she does it only one time ! While for someone who want read the code, specifying in the constructor self.thisPar = thisPar (or something else) is explicit and easier to understand, especially if you are not aware of the _register mechanism.

[pancetta]

I'd rather have one mechanism of "registering" attributes than two (which have to be done both). If you are clear about what the _register method does, I think there will be no confusion. Maybe with a more explicit name for this method (e.g. _make_attribute_and_register) this is not such a problem? The current proposal is not clear to me (if I were someone who wants to read the code): why do I need to store AND register? And what happens if I store but don't register?

[tlunet]

@pancetta I do like your idea of making the method name more explicit. I've implemented this in the last commit. Also on a side note : parameters are stored in set, as looking up an element is faster than in a list. But then the order they are given in the _makeAttributeAndRegister function does not matter, only the alphabetic order is used, see :

Since setting attribute of the class method is (theoretically) done only during the instantiation with __init__, it should not degrade that much performance to use list instead, which would allow to keep the parameter order in the params dictionary. Would you prefer that ?

[pancetta]

Any scenario where the order would matter? If not, a set it is.

[tlunet]

Looks good to me apart from minor things. Also, we should probably have either the helpers/pysdc_helper.py file or the core/Common.py file, but not both, since they have the same purpose. I don't care what name the file has, though.

helpers/pysdc_helper.py is supposed to disappear once we apply this "default argument" approach to every class and get rid of the FrozenClass type. But I would keep core/Common.py for base classes used as parent of core class, and are not used explicitly by people implementing specialized class in implementations (they'll only inherited from the Problem class, or others, that inherit from the common classes). helpers could however keep all the components that could be used later for specialized class in implementations (for instance, the get_finite_difference_matrix function).

[tlunet]

Just added the documentation (and also, did the same mistake as @brownbaerchen did before by merging master into my local v6 😅).

I used Numpy docstyle instead of Google docstyle. Yes it's produce longer documentation, but at least those can have more details and can be easily read from the code. Also, if necessary, most IDE's and editors have a way to hide docstring if necessary (for instance, if you are developing the class and don't care about documentation). Also, there are a lot of tools that generate automatically a template for the docstring from the signature of the function ...

Additional points :

• Constructor documentation is written in the class docstring. Its parameters are implicitly considered as attributes, and additional attributes (in particular properties) are given afterward in the Attributes section.
• Math formula can be added in latex format, and can be useful as most IDE have now a way to render docstrings with Sphinx (I'm sure there should be something like this in vim and pycharm ;). For instance with Spyder :
  
• Return argument have an explicit name and associated documentation. That way, it's easy to see by looking at the code what does this function returns and what this is.

It does (probably) not break the website documentation rendering. Any comment/thought ?

[brownbaerchen]

Alright! Good job! For a second I thought you already changed all the problems. Very annoying...

While we have to go through all the problems: I am in the process of adding a hook for storing the local error wrt to u_exact. However, for the local error we need to solve a single step starting from the initial conditions of the step, not of the run. For this reason I added two additional arguments u_init and t_init to the signature of u_exact for the van der Pol problem.

Currently this is the only problem supporting this and so I am unsure if everything associated should just go in my project folders rather than the implementations folder. But I do think this is a useful feature and I suggest to add the arguments to all u_exact functions and raise an error if you supply sth other than None if they don't support this feature.

Given that the scipy reference solution thing works rather well, I think this can easily be extended to more problems then just vdp later.

What do you think? Should I keep this to myself or should we add the extra lines to all classes?

[pancetta]

@brownbaerchen I'd be ok with that.

[pancetta]

Just added the documentation (and also, did the same mistake as @brownbaerchen did before by merging master into my local v6 😅).

I used Numpy docstyle instead of Google docstyle. Yes it's produce longer documentation, but at least those can have more details and can be easily read from the code. Also, if necessary, most IDE's and editors have a way to hide docstring if necessary (for instance, if you are developing the class and don't care about documentation). Also, there are a lot of tools that generate automatically a template for the docstring from the signature of the function ...

Additional points :

* Constructor documentation is written in the class docstring. Its parameters are implicitly considered as attributes, and additional attributes (in particular properties) are given afterward in the `Attributes` section.

* Math formula can be added in latex format, and can be useful as most IDE have now a way to render docstrings with Sphinx (I'm sure there should be something like this in vim and pycharm ;). For instance with Spyder :
  ![grafik](https://user-images.githubusercontent.com/12183839/212753213-7917788e-2605-42ba-97cf-3e5d6b5015d4.png)

* Return argument have an explicit name and associated documentation. That way, it's easy to see by looking at the code what does this function returns and what this is.

It does (probably) not break the website documentation rendering. Any comment/thought ?

Looks good indeed, but I worry that we're going to mess up things. There is no consistency in the documentation anymore, if we have (1) rst and md files as readmes and (2) NumPy and Google docstyle mixed throughout the code. I agree that some choices may not be optimal, but they are not dealbreakers. So, I'd rather not mix up different docstyles.

[tlunet]

Just added the documentation (and also, did the same mistake as @brownbaerchen did before by merging master into my local v6 sweat_smile).
I used Numpy docstyle instead of Google docstyle. Yes it's produce longer documentation, but at least those can have more details and can be easily read from the code. Also, if necessary, most IDE's and editors have a way to hide docstring if necessary (for instance, if you are developing the class and don't care about documentation). Also, there are a lot of tools that generate automatically a template for the docstring from the signature of the function ...
Additional points :

* Constructor documentation is written in the class docstring. Its parameters are implicitly considered as attributes, and additional attributes (in particular properties) are given afterward in the `Attributes` section.

* Math formula can be added in latex format, and can be useful as most IDE have now a way to render docstrings with Sphinx (I'm sure there should be something like this in vim and pycharm ;). For instance with Spyder :
  ![grafik](https://user-images.githubusercontent.com/12183839/212753213-7917788e-2605-42ba-97cf-3e5d6b5015d4.png)

* Return argument have an explicit name and associated documentation. That way, it's easy to see by looking at the code what does this function returns and what this is.

It does (probably) not break the website documentation rendering. Any comment/thought ?

Looks good indeed, but I worry that we're going to mess up things. There is no consistency in the documentation anymore, if we have (1) rst and md files as readmes and (2) NumPy and Google docstyle mixed throughout the code. I agree that some choices may not be optimal, but they are not dealbreakers. So, I'd rather not mix up different docstyles.

This is just a suggestion for v6. We will anyway have to rework many classes with this idea of default parameters and _register approach intended to replace the FrozenClass (not allowing addition of new attribute as FrozenClass does can also be added ...). Also, if we tend to apply the new contribution guidelines for v6, there will also some additional rework. In the meantime, going through a rework of all class documentation won't be that much in comparison to what will already have to be done (and there is no timer on a v6 release, right ?).

So the point here is : Numpy of Google docstyle, what do you think is best ? (I used Numpy style here to have a comparison ...)

And about the mix rst and md : this is also a temporary solution that will have to be fixed, cf #250

[tlunet]

Alright! Good job! For a second I thought you already changed all the problems. Very annoying...

While we have to go through all the problems: I am in the process of adding a hook for storing the local error wrt to u_exact. However, for the local error we need to solve a single step starting from the initial conditions of the step, not of the run. For this reason I added two additional arguments u_init and t_init to the signature of u_exact for the van der Pol problem.

Currently this is the only problem supporting this and so I am unsure if everything associated should just go in my project folders rather than the implementations folder. But I do think this is a useful feature and I suggest to add the arguments to all u_exact functions and raise an error if you supply sth other than None if they don't support this feature.

Given that the scipy reference solution thing works rather well, I think this can easily be extended to more problems then just vdp later.

What do you think? Should I keep this to myself or should we add the extra lines to all classes?

I do aggree (too), see Discussion #253

[pancetta]

Agreed, agreed. I just wanted to make sure we're not mixing up too many things. Concerning docstyles: I'm ok with either of these. NumPy's would be probably more fitting from what I see and it would give us the chance of checking all docstrings for consistency and correctness.

[pancetta]

helper

Concerning helpers: the fewer we have the better. If you see a way to get rid of some of then by integrating them into some core functionality, feel free!

[tlunet]

@brownbaerchen @pancetta can we merge this into v6 ? So I can focus on #254 (see all steps proposed in #255 )

[pancetta]

@brownbaerchen @pancetta can we merge this into v6 ? So I can focus on #254 (see all steps proposed in #255 )

LGTM