openturns / openturns

Uncertainty treatment library
http://openturns.github.io/openturns/latest/index.html
Other
236 stars 93 forks source link

Methods of Field have wrong name #1463

Open mbaudin47 opened 4 years ago

mbaudin47 commented 4 years ago

Firstly, the Field class has a getOutputDimension method.

In the following example, we consider a 2D domain, with 1D field values.

import openturns as ot
myVertices = [[0.0, 0.0], [1.0, 0.0], [1.0, 1.0], [1.5, 1.0], [2.0, 1.5], [0.5, 1.5]]
mySimplicies = [[0,1,2], [1,2,3], [2,3,4], [2,4,5], [0,2,5]]
myMesh = ot.Mesh(myVertices, mySimplicies)
myValues = [[2.0],[2.5],[4.0], [4.5], [6.0], [7.0]]
myField = ot.Field(myMesh, myValues)
dimension = myField.getOutputDimension() # Returns 1

This is inconsistent with the ProcessSample class, which has a getDimension method. This is why the getOutputDimension of the Field class should be renamed getDimension.

Secondly, the Field class has a getInputDimension method which returns the domain dimension. This does not correspond to the ProcessSample class, which does not have such method. The getInputDimension method is unnecessary: this produces the same result as getMesh().getDimension(). Hence, the getInputDimension method should be removed from the API.

jschueller commented 4 years ago

For 1. I think we decided on doing exactly the opposite a while ago : )

regislebrun commented 4 years ago

If we go for getMesh().getDimension(), while not using getValues().getDimension()? The objective of these methods is to get a direct access to information with no knowledge of the internal structure of the object. If we think about a field as a discretized Function, it sounds reasonable to have an input dimension and an output dimension (even if this last one is named simply dimension), and the corresponding accessors, no?

sofianehaddad commented 4 years ago

For the first point, maybe ProcessSample.getDimension is a bit confusing. As ProcessSample might be seen as a collection of Field, we might have :

The methods become more explicit. But it is differ comparing to other places in the API. Do we need to have exactly the same method names in the API ?

For the second point, fully disagreed about removing methods. Even if it is a redirection, we need these methods and do not add some complexity to end users

mbaudin47 commented 4 years ago

Ok let's keep the existing methods, but not their names.

To sum my point, here is the current situation:

My favorite API would be following Sofiane's ideas :

It is clear on the content and uniform across classes.

sofianehaddad commented 4 years ago

And to be coherent with the rest of the API we should also rename CovarianceModel's methods. I think we made the exercise few years ago and agreed that time on the current API. So should we go for renaming the all methods?

mbaudin47 commented 4 years ago

I cannot see the consistency of the present API, where the same idea has a different name for a Field or for a ProcessSample, except if someone can defend the position that the container should have a different naming system than what is contained. However, I admit that there is a cost at changing the names, so that we should minimize the number of required changes.

Let me include the CovarianceModel into the discussion. The current API is:

A consistent API would then be the following, based on the getInputDimension and getOutputDimension method names:

This leaves unchanged most method names, creates a new method and fixes a local inconsistency for ProcessSample. Deal ?

regislebrun commented 4 years ago

Deal! It is in agreement with the Process API too.

jschueller commented 4 years ago

Ok, this is acceptable.

jschueller commented 4 years ago

After some thoughts, it feels a bit weird to have getOutputDimension for ProcessSample and getDimension in Sample

mbaudin47 commented 4 years ago

After some thoughts, it feels a bit weird to have getOutputDimension for ProcessSample and getDimension in Sample

It might seem so, but it is not:

(I know that you have this in mind, but this clarifies the discussion.)

Therefore, it does introduce a sort of mismatch, but this difference clarifies an ambiguity: this is why it can be considered as being useful. The main issue here is our lack of vocabulary, so that we have just one name "dimension" for different purposes: one single name for two different ideas make things confusing.