Keep the names of the variables with the table

[avdg81]

📝 Description
The names of the variables X and Y of a table were read (from the mdpa file), but not stored. Now they are. This allows the user of a table to give meaning to the stored values and act differently if needed.

🆕 Changelog

• Added (private) data members to keep the names of X and Y with the table. Also added getters and setters for these.
• When a table is being read from an mdpa file, no longer ignore the names of X and Y, but store them.
• Removed some commented out code.
• Use compiler-generated special member functions (e.g. copy constructors) rather than reimplementing the default behavior.
• Single-argument constructors should be explicit.
• Use nested namespace definitions to improve readability.
• Removed needless indentation and some redundant comments.
• Added a unit test to cover the new functionality.

[loumalouomega]

FYI @KratosMultiphysics/altair

[loumalouomega]

My only comment is that maybe is better to use directly variables (which include names), because variables provide much mor info @KratosMultiphysics/technical-committee

[loumalouomega]

Blocking to avoid accidental merge

[loumalouomega]

My only comment is that maybe is better to use directly variables (which include names), because variables provide much mor info @KratosMultiphysics/technical-committee

And the size in that cae would be 2 doubles (2 pointers)

[matekelemen]

Blocking to avoid accidental merge

@loumalouomega "Accidental" merges don't happen. A PR cannot be merged without an approval from someone with the correct privileges.

You should only block a PR if you spot something that's a dealbreaker for you, but you must explain the problem you have in that case, instead of referring to *"accidental merge"*s.

Now that you've blocked the PR, the only way you can unblock it is to approve it, which means that you'll have to track the progress of this PR and properly review it once changes are made. This can lead to a situation in which others may have to wait for you until you find the time to take care of your duties here.

[loumalouomega]

@technical-committee usually blocks "to avoid accidental merge"

[loumalouomega]

@technical-committee usually blocks "to avoid accidental merge"

This may happen if someone approves before @technical-committee review and it is merged

[loumalouomega]

Blocking to avoid accidental merge

@loumalouomega "Accidental" merges don't happen. A PR cannot be merged without an approval from someone with the correct privileges.

You should only block a PR if you spot something that's a dealbreaker for you, but you must explain the problem you have in that case, instead of referring to *"accidental merge"*s.

Now that you've blocked the PR, the only way you can unblock it is to approve it, which means that you'll have to track the progress of this PR and properly review it once changes are made. This can lead to a situation in which others may have to wait for you until you find the time to take care of your duties here.

https://github.com/search?q=repo%3AKratosMultiphysics%2FKratos+accidental+merge&type=pullrequests

[loumalouomega]

Blocking to avoid accidental merge

@loumalouomega "Accidental" merges don't happen. A PR cannot be merged without an approval from someone with the correct privileges.

You should only block a PR if you spot something that's a dealbreaker for you, but you must explain the problem you have in that case, instead of referring to *"accidental merge"*s.

Now that you've blocked the PR, the only way you can unblock it is to approve it, which means that you'll have to track the progress of this PR and properly review it once changes are made. This can lead to a situation in which others may have to wait for you until you find the time to take care of your duties here.

BTW, I added a comment saying that I would like to discuss if variable is a better alternative to string

[matekelemen]

@technical-committee usually blocks "to avoid accidental merge"

This may happen if someone approves before @technical-committee review and it is merged

Blocking the PR is not a solution to that problem. That's the job of code owners.

[avdg81]

My only comment is that maybe is better to use directly variables (which include names), because variables provide much mor info @KratosMultiphysics/technical-committee

I have considered to use variables directly, but I see two problems there:

1. There may already exist mdpa files where names have been used that have no corresponding variable. Obviously, that can only be the case when someone has edited the file manually, but this problem wouldn't exist if you use strings.
2. Last week I have tried really hard to find a way to retrieve the variable that corresponds to a given name. Put simply, the problem is as follows. Each kind of variable (e.g. Variable<double>) has its own mapping from name to the corresponding variable kept by a KratosComponents class (there are several of them, varying by variable type). If all you know is the name, you would need to search all mappings to find what you need. That is not particularly elegant. There is also a special kind of KratosComponents for VariableData, which keeps a mapping from name to the base class pointers of the variables, and which contains all variable names. The base class, however, lacks the concrete type information. So unless you are aware of a sound way to find the variable only given its name, I don't see how we can implement this in an elegant manner. Also, what should be the return type of the getter if the variable types may differ? If you have a working example of that I would love to learn more about it. Thanks.

[ddiezrod]

Blocking the PR is not a solution to that problem. That's the job of code owners.

I understand your point @matekelemen . The problem with code owners is that if we add for example the @KratosMultiphysics/technical-committee as code owner of kratos core, it will be much harder to get anything merged. Blocking a PR is the only way I see now to avoid "accidental merges" (someone merging something because they think its not an important change but it might actually be). I'd like to hear how you think this should be done (maybe in a new issue so everyone can comment)

[loumalouomega]

@technical-committee usually blocks "to avoid accidental merge"

This may happen if someone approves before @technical-committee review and it is merged

Blocking the PR is not a solution to that problem. That's the job of code owners.

Well, I have shared to you a search result that shows like this is an extended practice to avoid potential API changes

[loumalouomega]

@avdg81 sorry but a recent change have conflicted this (it justb changes the C++ tests macros)

[loumalouomega]

I remove my block, but wait until @KratosMultiphysics/technical-committee agrees

[avdg81]

I remove my block, but wait until @KratosMultiphysics/technical-committee agrees

@loumalouomega: Sure, I will wait.

I would appreciate it very much if someone from the @KratosMultiphysics/technical-committee would be so kind to review this PR. Many thanks in advance!

[rubenzorrilla]

We had a brief look to this in the @KratosMultiphysics/technical-committee. @pooyan-dadvand will review and take care of this one.

[avdg81]

We had a brief look to this in the @KratosMultiphysics/technical-committee. @pooyan-dadvand will review and take care of this one.

That's great. Thank you very much for the work done so far. I'm looking forward to the review of @pooyan-dadvand.

[pooyan-dadvand]

I think this is a good addition. Nevertheless, I am not sure why are you deleting the explicit assignment operators and copy constructors? To my understanding, it is cleaner to explicitly setting them to default.

[pooyan-dadvand]

Some more comments after reviewing:

I think it would be better adding the names directly to the original template class and not only to the double specialization.

Another possibility is to add the variables with the type of the template. This would have an extra benefit that we can then use them in the processes to get the values from the nodes.

I should add that we had the pointer to variables in the first version of template but we had to took it off for using a table with a variable component. Now the variable component is the same type as a normal variable. So, we may turn back to the original idea.

[avdg81]

I think this is a good addition. Nevertheless, I am not sure why are you deleting the explicit assignment operators and copy constructors? To my understanding, it is cleaner to explicitly setting them to default.

Dear @pooyan-dadvand, first of all thank you very much for taking the time to review my PR. I'm glad to read that you think this is a good addition.

Regarding the removal of the copy assignment operators and copy constructors, please let me explain my considerations. First of all, there was a difference in the copy semantics of class template Table and the template specialization Table<double, double>. The former did not have consistent copy operations: copy assignment was supported, but copy construction was not (since its declaration was private). To me as a programmer, that is very confusing. It means that I cannot do this:

// Assume object `foo` is a `Table<int, int>` object
Table<int, int> bar = foo; // compilation error: copy constructor is disabled (deleted)

But I can do this:

// Assume object `foo` is a `Table<int, int>` object
Table<int, int> bar; // the class is default-constructible
bar = foo; // ok: copy assignment is supported

On the other hand, when we consider the template specialization Table<double, double>, both copy operations are supported and consistent in their behavior, which makes sense. (That is, they are either both supported and expose similar behavior, or they are both deleted.)

When I looked at the implementation of the supported copy operations, I noticed that all they do is a member-wise copy. Which is exactly what the compiler would generate for you if you hadn't defined them. Assuming the implementation as it was, when new data members are added to the class, the programmer must not forget to handle them in the copy operations. This is OK if a member-wise copy is not the correct semantics for the class. But in this case, we actually want member-wise copy operations. In that case, it's better to let the compiler handle this. Since I don't see any problem in the ability to copy Table objects (e.g. no pointer ownership issues), I decided to let the compiler generate these operations for us.

And last but not least, also the C++ Core Guidelines suggest to let the compiler generate the special member functions whenever appropriate. For further background, please refer to sections "C.20: If you can avoid defining default operations, do" (also known as the rule of zero) and "C.22: Make default operations consistent".

If you feel that I have misunderstood your point, please let me know. Thanks.

(I will now move on to respond to your second reply.)

[avdg81]

Some more comments after reviewing:

I think it would be better adding the names directly to the original template class and not only to the double specialization.

Yes, I agree. I think I've missed that one when I made the extensions. Thanks for pointing that out. When you agree that we should use strings rather than variables (see my reply below) I will add it to the class template and add a unit test for it as well.

Another possibility is to add the variables with the type of the template. This would have an extra benefit that we can then use them in the processes to get the values from the nodes.

I should add that we had the pointer to variables in the first version of template but we had to took it off for using a table with a variable component. Now the variable component is the same type as a normal variable. So, we may turn back to the original idea.

Two things came to my mind in this regard:

1. Your suggestion to use variables rather than just names (i.e. strings) limits the scope of identifiers (i.e. the supported strings in the definition of a table) to known variables only. In many cases, this won't be a problem. However, what if I want to use an auxiliary variable that hasn't been defined in Kratos? With your suggestion, the only option I have is to add the auxiliary variable to Kratos as well. Even if I don't use it in any other way. The approach with the names is more flexible in that case.
2. I don't see how to find the Variable instance corresponding to a given name (i.e. string) in an elegant way. I am aware that the template specialization KratosComponents<VariableData> offers a map (msComponents) from a name (string) to a VariableData pointer, but those lack the concrete variable type (e.g. double). (Note that class template Variable is derived from class VariableData, but only the former has knowledge about the concrete kind of variable.) Each concrete Variable class has its own map from a name to a Variable of that particular type. So I could search all those maps and try to find the variable that I need (which I feel is a rather ugly solution (it requires enumerating all kinds of variables) that needs to be updated whenever a new kind of variable is added => error prone!). Otherwise, we might attempt to do some explicit casting, but then again, we need to iterate over all kinds of variables. So unless I overlook a nice, compact and comprehensive solution that doesn't need to be extended when new variable types are added, I don't see how to solve this in an elegant way. If you have a good idea, could you please be more explicit about how you would implement it? Perhaps provide a code snippet to demonstrate what you mean? Thanks.

[pooyan-dadvand]

Thanks for the explanation!

Regarding the removal of the copy assignment operators and copy constructors, please let me explain my considerations. First of all, there was a difference in the copy semantics of class template Table and the template specialization Table<double, double>. The former did not have consistent copy operations: copy assignment was supported, but copy construction was not (since its declaration was private). To me as a programmer, that is very confusing. It means that I cannot do this:

Fully agree that it is not correct and it would be better the unified one!

When I looked at the implementation of the supported copy operations, I noticed that all they do is a member-wise copy. Which is exactly what the compiler would generate for you if you hadn't defined them. Assuming the implementation as it was, when new data members are added to the class, the programmer must not forget to handle them in the copy operations. This is OK if a member-wise copy is not the correct semantics for the class. But in this case, we actually want member-wise copy operations. In that case, it's better to let the compiler handle this. Since I don't see any problem in the ability to copy Table objects (e.g. no pointer ownership issues), I decided to let the compiler generate these operations for us.

And last but not least, also the C++ Core Guidelines suggest to let the compiler generate the special member functions whenever appropriate. For further background, please refer to sections "C.20: If you can avoid defining default operations, do" (also known as the rule of zero) and "C.22: Make default operations consistent".

I think that I didn't explain myself correctly. I agree that using old style copy constructors for member-wise copying is not good anymore. I was referring to explicitly define them using =default instead of simply removing it. I fully agree that using =default is better that what was there. (please note that the original code was before C++11 which supports such semantic)

[pooyan-dadvand]

1. Your suggestion to use variables rather than just names (i.e. strings) limits the scope of identifiers (i.e. the supported strings in the definition of a table) to known variables only. In many cases, this won't be a problem. However, what if I want to use an auxiliary variable that hasn't been defined in Kratos? With your suggestion, the only option I have is to add the auxiliary variable to Kratos as well. Even if I don't use it in any other way. The approach with the names is more flexible in that case.

The argument of arbitrary name is very valid and we can go with the current implementation.

2. I don't see how to find the Variable instance corresponding to a given name (i.e. string) in an elegant way. I am aware that the template specialization KratosComponents<VariableData> offers a map (msComponents) from a name (string) to a VariableData pointer, but those lack the concrete variable type (e.g. double). (Note that class template Variable is derived from class VariableData, but only the former has knowledge about the concrete kind of variable.) Each concrete Variable class has its own map from a name to a Variable of that particular type. So I could search all those maps and try to find the variable that I need (which I feel is a rather ugly solution (it requires enumerating all kinds of variables) that needs to be updated whenever a new kind of variable is added => error prone!). Otherwise, we might attempt to do some explicit casting, but then again, we need to iterate over all kinds of variables. So unless I overlook a nice, compact and comprehensive solution that doesn't need to be extended when new variable types are added, I don't see how to solve this in an elegant way. If you have a good idea, could you please be more explicit about how you would implement it? Perhaps provide a code snippet to demonstrate what you mean? Thanks.

I see your point, but the idea was to have the type of the variable inside the template of the table. Nevertheless, your argument for arbitrary name is valid and we can go with the string implementation.

[sunethwarna]

Here are my two cents on this problem of identifying variables from strings. I also encountered it many times.

Are we going for arbitrary types of variables? I got the feeling that we have only limited types of variables such as bool, int, double, array_1d<double, 3>, array_1d<double, 4>, array_1d<double, 6>, array_1d<double, 9>, Vector, Matrix. If that is the case, I usually use the following code snippet:

template <class TComponentType>
bool DoSomethingWithVariable(const std::string& rVarName) {
   if (KratosComponents<TComponentType>::Has(rVarName)) {
      const auto& r_var = KratosComponents<TComponentType>::Get(rVarName);
      // do something with var.
      return true;
   }

   return false;
}

template<class... TComponentTypes>
class Doer
{
   static bool DoSomething(const std::string& rVarName) {
      return (... || (DoSomethingWithVariable<TComponentTypes>(
                        rVarName)));
   }
}

// use case
Doer<Variable<double>,
     Variable<int>>::DoSomething("STEP");

It is not ideal, but it does the job.

[avdg81]

I think that I didn't explain myself correctly. I agree that using old style copy constructors for member-wise copying is not good anymore. I was referring to explicitly define them using =default instead of simply removing it. I fully agree that using =default is better that what was there. (please note that the original code was before C++11 which supports such semantic)

I think that, as suggested by "C.20: If you can avoid defining default operations, do" of the C++ Core Guidelines, the preferred way should be to apply the rule of zero. Strictly speaking, the rule of zero doesn't apply here, since we must declare a (default) virtual destructor (class Table has a few virtual member functions; these need not be virtual since there are no classes that derive from Table; but that is a different discussion). Still, I feel it's the cleanest and simplest semantics to not user-declare the other special member functions (that includes declaring them as =default). That style is also demonstrated by an example from "C.21: If you define or =delete any copy, move, or destructor function, define or =delete them all", where a destructor needs to be declared just to make it virtual (which is then defined as defaulted):

class AbstractBase {
public:
    virtual void foo() = 0;  // at least one abstract method to make the class abstract
    virtual ~AbstractBase() = default;
    // ...
};

However, if you insist on having =default-declared copy operations, I will add them. In that case, I would suggest to also add =default-declared move operations (to comply with the rule of five). Could you please let me know what your preferred option is? Thanks.

If, on the other hand, you feel that no further changes are required, could you please approve this PR?

[avdg81]

1. Your suggestion to use variables rather than just names (i.e. strings) limits the scope of identifiers (i.e. the supported strings in the definition of a table) to known variables only. In many cases, this won't be a problem. However, what if I want to use an auxiliary variable that hasn't been defined in Kratos? With your suggestion, the only option I have is to add the auxiliary variable to Kratos as well. Even if I don't use it in any other way. The approach with the names is more flexible in that case.

The argument of arbitrary name is very valid and we can go with the current implementation.

2. I don't see how to find the Variable instance corresponding to a given name (i.e. string) in an elegant way. I am aware that the template specialization KratosComponents<VariableData> offers a map (msComponents) from a name (string) to a VariableData pointer, but those lack the concrete variable type (e.g. double). (Note that class template Variable is derived from class VariableData, but only the former has knowledge about the concrete kind of variable.) Each concrete Variable class has its own map from a name to a Variable of that particular type. So I could search all those maps and try to find the variable that I need (which I feel is a rather ugly solution (it requires enumerating all kinds of variables) that needs to be updated whenever a new kind of variable is added => error prone!). Otherwise, we might attempt to do some explicit casting, but then again, we need to iterate over all kinds of variables. So unless I overlook a nice, compact and comprehensive solution that doesn't need to be extended when new variable types are added, I don't see how to solve this in an elegant way. If you have a good idea, could you please be more explicit about how you would implement it? Perhaps provide a code snippet to demonstrate what you mean? Thanks.

I see your point, but the idea was to have the type of the variable inside the template of the table. Nevertheless, your argument for arbitrary name is valid and we can go with the string implementation.

Thanks for your consideration and for agreeing with the proposed implementation.

[avdg81]

Here are my two cents on this problem of identifying variables from strings. I also encountered it many times.

Are we going for arbitrary types of variables? I got the feeling that we have only limited types of variables such as bool, int, double, array_1d<double, 3>, array_1d<double, 4>, array_1d<double, 6>, array_1d<double, 9>, Vector, Matrix. If that is the case, I usually use the following code snippet:

template <class TComponentType>
bool DoSomethingWithVariable(const std::string& rVarName) {
   if (KratosComponents<TComponentType>::Has(rVarName)) {
      const auto& r_var = KratosComponents<TComponentType>::Get(rVarName);
      // do something with var.
      return true;
   }

   return false;
}

template<class... TComponentTypes>
class Doer
{
   static bool DoSomething(const std::string& rVarName) {
      return (... || (DoSomethingWithVariable<TComponentTypes>(
                        rVarName)));
   }
}

// use case
Doer<Variable<double>,
     Variable<int>>::DoSomething("STEP");

It is not ideal, but it does the job.

Thanks for sharing your thoughts. Like you said, it is not ideal, but it does the job.

[pooyan-dadvand]

Thank you for the implementation and also your clear explanation!

[avdg81]

Thank you for the implementation and also your clear explanation!

Well, thank you for your thorough review and your questions :-). I really appreciate that.

By the way, I have corrected a thing that you pointed out earlier. It was about adding the variable names to the class template Table too, not just the template specialization Table<double, double>. You may want to have a quick look at that to verify that it is correct now. Due to the new commit your approval has been withdrawn automatically. If you agree with the implementation as it is now, may I ask you to approve it once more? Thanks for your time and your constructive comments!

[avdg81]

@pooyan-dadvand: Just a friendly reminder that this PR needs your approval (once more) in order to be merged (assuming you agree with the latest changes). I only implemented one of your previous suggestions after your approval (see my previous comment), which means it was automatically withdrawn. My apologies for the inconvenience and thanks in advance for your help.

[pooyan-dadvand]

Thank you again!