Adding air specific mass to environment description

[mcharlou]

Description

This pull request adds the air specific mass to the environment description in Xdyn, as well as the means to specify the value in the input.

Related Issue

Issue #13

Motivation and Context

See Issue #13

How Has This Been Tested?

Everything was compiled and tested successfully with the default Debian 10 image from Sirehna, according to the main MakeFile. However other platforms were not tested, but the CI engine should do that anyway.

All the complete YAML inputs for unit tests were modified to include the key 'air rho' in the 'environmental constants' section, because this input was made mandatory. This effectively make all previous working input files obsolete. If that is not acceptable, the air specific mass could be made an optional input, but this would mean giving it a different treatment than the other environmental constants, which is debatable.

[codecov-io]

Codecov Report

Merging #14 (3008029) into master (4c4db1c) will increase coverage by 0.00%.
The diff coverage is 92.30%.

@@           Coverage Diff           @@
##           master      #14   +/-   ##
=======================================
  Coverage   87.08%   87.08%           
=======================================
  Files         222      222           
  Lines       11073    11084   +11     
=======================================
+ Hits         9643     9653   +10     
- Misses       1430     1431    +1     

Impacted Files	Coverage Δ
code/test_data_generator/src/yaml_data.cpp	93.05% <ø> (ø)
..._parser/unit_tests/src/SimulatorYamlParserTest.cpp	100.00% <ø> (ø)
code/core/src/EnvironmentAndFrames.cpp	76.92% <87.50%> (+3.23%)	⬆️
code/core/src/SimulatorBuilder.cpp	88.02% <100.00%> (+0.08%)	⬆️
...data_structures/src/YamlEnvironmentalConstants.cpp	100.00% <100.00%> (ø)
...ml_parser/src/external_data_structures_parsers.cpp	83.52% <100.00%> (+0.28%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c4db1c...3008029. Read the comment docs.

[Gjacquenot]

Thank again for this pull request Moran!

We can't accept directly this pull request as it breaks all existing simulations.

As discussed with Charles-Édouard, if this new parameter is going to be used for several force models, then it should be an optional environment parameter. Otherwise, it should be associated to a specific force model.

I guess it is the first option: so we would appreciate if you could update your pull request and make the parameter optional.

[mcharlou]

Allright, I was half-expecting that, so I'll quickly make the correction.

What do you think is best to solve this:

• A default value of 1.225kg/m3, and the optional input overwrites it, or
• A non-physical default value of 0 or -1, and the force models that use it have to check that it has been overwritten ?

Thank you for your feedback !

[mcharlou]

I have chosen the first option for the moment being.

The problem with the second option is that it forces the force models to check for the value of rho_air, which should not be their responsibility.

A third option would be to make the simulation builder check that rho_air has been provided in the input if any force model using it is also in the simulation. However this would introduce unnecessary complexity in my opinion...

Please let me know if you are OK with the current solution or if you prefer another!

[CharlesEdouardCady]

• A default value of 1.225kg/m3, and the optional input overwrites it, or
• A non-physical default value of 0 or -1, and the force models that use it have to check that it has been overwritten ?

How many models use this?

• If it's only one (or one class of models, e.g. if you added a section to environment models), then I would argue it's best to leave the key in the model's YAML
• If several models use it concurrently (if you need several models using the same value of rho) then it should be an optional value in then environmental constants section. In that case, I would add a bost/optional to YamlEnvironmentalConstants.

[CharlesEdouardCady]

I have chosen the first option for the moment being.

The problem with the second option is that it forces the force models to check for the value of rho_air, which should not be their responsibility.

A third option would be to make the simulation builder check that rho_air has been provided in the input if any force model using it is also in the simulation. However this would introduce unnecessary complexity in my opinion...

Please let me know if you are OK with the current solution or if you prefer another!

I think it's the constructor's responsibility to check its input parameters: it should not assume they are valid (defensive programming - this is C++, not Erlang! 😉), especially if those parameters come from the user, and should throw a descriptive exception if they're not valid.

[mcharlou]

If several models use it concurrently (if you need several models using the same value of rho) then it should be an optional value in then environmental constants section. In that case, I would add a bost/optional to YamlEnvironmentalConstants.

Indeed, the idea is that several models will use It. The issue is not so much in YamlEnvironmentalConstants as in EnvironmentAndFrames: it's OK if the user provided a value. But what do we do if the user did not provide a value? If there is no force model using it, that is not a problem, but as soon as it's the case we must either raise an error or assume a value for rho_air.
So I can make it 'boost-optional' in YamlEnvironmentalConstants, but when building EnvironmentAndFrames we still have to make a choice...

I think it's the constructor's responsibility to check its input parameters: it should not assume they are valid (defensive programming - this is C++, not Erlang! wink), especially if those parameters come from the user, and should throw a descriptive exception if they're not valid.

I wasn't thinking about an invalid value from the user (in which case a simple check in the constructor is enough, as it is done for the other environmental constants). I was thinking of a way for the force models that want to use rho_air to be aware that it has not been set by the user, but at the same time providing the same simple access as for the other environmental variables (i.e. env.rho_air).

[CharlesEdouardCady]

Indeed, the idea is that several models will use It.

The question is whether several models use it at the same time: we want to avoid declaring the same value in several places. If only wind models use it and we have a maximum of one wind force model in use in a simulation, then I would argue it is better to put the air density parameter in that force model's parameters (and if there are several such force models, they could use the same parser). If there are indeed several models used at the same time that need the same value, then we should put it in the environmental constants section.

If there is no force model using it, that is not a problem, but as soon as it's the case we must either raise an error or assume a value for rho_air.

I would definitely raise an error and not assume a value by default: I prefer all values to be explicitly given by the user if possible.
I don't think it's a problem if the exception is raised from the force model (and not the environment parser) as there is only one reason the value would be missing (it isn't in the YAML) and it's only a problem if trying to use a model that needs it.

I was thinking of a way for the force models that want to use rho_air to be aware that it has not been set by the user, but at the same time providing the same simple access as for the other environmental variables (i.e. env.rho_air).

I think boost::optional might be less cumbersome than you think, but in any case you could just keep the boost::optional in EnvironmentAndFrames and copy the value inside the wind model's constructor (to a standard double) or throw if the value is not defined.

[mcharlou]

I did as you suggested and used boost::optional for the new environmental constant rho_air. However it is only a nicer way of using an invalid value (e.g. -1) to tell the user (force model) that no input was provided.

I solved the problem of not giving the users (force models) the responsibility of throwing when rho_air is not initialized by giving this responsibility to EnvironmentAndFrames. However this prevents an access to rho_air equivalent to the other environmental constants (i.e. direct access to public member), the access is instead given with the method 'get_rho_air()'.

Sorry for the force-push, I went back a few commits to make the changes and I wanted to keep a clean history.

[CharlesEdouardCady]

it is only a nicer way of using an invalid value (e.g. -1) to tell the user (force model) that no input was provided.

Not really. First of all, it's not the same type so it forces you to check the value is available before using it. Second, it is a more general way of dealing with missing values: this case is very simple because a valid value can easily be distinguished from an invalid one. However, if you were dealing with a bank account balance for example, it's less clear what an "invalid value" might be, so you introduce a risk. By the way, C++17 introduced std::optional which works the same way.
Optional values can be seen as 'Maybe' monad and that means you can deal with them in an attractive way:

std::optional<image_view> get_cute_cat (image_view img) {
    return find_cat(img)
           .and_then(make_eyes_sparkle)
           .map(make_smaller)
           .map(add_rainbow);
}

where make_eyes_sparkle, make_smaller and add_rainbow all operate on image_view, which means you're essentially only writing the "happy case" and you let the Maybe Monad do its magic. This example was taken from here, by the way)

I solved the problem of not giving the users (force models) the responsibility of throwing when rho_air is not initialized by giving this responsibility to EnvironmentAndFrames. However this prevents an access to rho_air equivalent to the other environmental constants (i.e. direct access to public member), the access is instead given with the method 'get_rho_air()'.

That's alright: it doesn't have the same status as it can be missing, so I don't think it should be dealt with the same way.

Sorry for the force-push, I went back a few commits to make the changes and I wanted to keep a clean history.

No problem!

[CharlesEdouardCady]

Defining both a setter & a getter essentially makes the variable public: why do you need the setter?

[mcharlou]

I need the setter to ensure the force models do not use rho_air directly when they want its value, because if it is unavailable it will raise an exception from boost::optional, and the user won't be able to understand that 'air rho' is missing in the input. By forcing the force models to get it with the getter, I ensure the right exception is thrown. The setter is needed because of the way SimulatorBuilder builds EnvironmentAndFrames.

[mcharlou]

Basically, the member rho_air is public (like all other members), but its read-access is wrapped with the proper exception thrown if the value is not available.

[mcharlou]

Not really. First of all, it's not the same type so it forces you to check the value is available before using it. Second, it is a more general way of dealing with missing values: this case is very simple because a valid value can easily be distinguished from an invalid one. However, if you were dealing with a bank account balance for example, it's less clear what an "invalid value" might be, so you introduce a risk. By the way, C++17 introduced std::optional which works the same way.

Thank you for your explanations! I must not have been clear enough in my answers to make myself understood. I am well aware of the improvement that 'optional' brings to the table for dealing with variables whose values might not be available. What I meant is that it does not make a difference from an invalid value (e.g. -1) in this case from an OOP structural standpoint. It is much more elegant and practical than using an invalid value, which is why I 100% agree that it should be used in this case.

However my main problem is 'onto which class falls the responsibility of throwing when the value is not available', which 'optional' does not solve. In my opinion it should not be the responsibility of the force model (see my answer to you code review). Ideally this should fall onto the building mechanisms (so that an exception is thrown as early as the building process), but this would be much more complicated. Giving this responsibility to EnvironmentAndFrames, also it refers to the input method (which should not be the case because EnvironmentAndFrames should not be aware of the input method), seems an acceptable compromise for now. What do you think ?

[CharlesEdouardCady]

However my main problem is 'onto which class falls the responsibility of throwing when the value is not available', which 'optional' does not solve. In my opinion it should not be the responsibility of the force model (see my answer to you code review). Ideally this should fall onto the building mechanisms (so that an exception is thrown as early as the building process), but this would be much more complicated. Giving this responsibility to EnvironmentAndFrames, also it refers to the input method (which should not be the case because EnvironmentAndFrames should not be aware of the input method), seems an acceptable compromise for now. What do you think ?

I think rho_air is const so in the wind force model's constructor you could initialize a rho_air member (which would have type double and not boost:: (or std::) optional by calling EnvironmentAndFrames::get_rho_air, which will throw if the value has not been defined.

Of course, if rho_air needs to vary during the simulation, we need to handle it entirely differently (probably by augmenting the state of the system) and not treat it as a parameter.

[CharlesEdouardCady]

I think there is an indentation problem here.

[mcharlou]

I'm gonna take a look at Eclipse's parameters, but I mainly auto-align using the IDE. As far as I understand there is a debate in the development community about tabulations and spaces for indentation and/or alignment. I'm ready to align myself to your standards, however I noticed a number of inconsistencies in indentation in Xdyn so I wasn't sure there actually was a standard there...

[CharlesEdouardCady]

air rho could be renamed "air density" perhaps? We probably shouldn't have given Greek letter names to the other parameters in the first place...

In the code, you use rho_airbut the YAML key is air rhowhich seems inconsistent.

[mcharlou]

I agree that using greek letters isn't optimal, but I followed your convention. In formulas I would have written this rho_air (with the greek symbol as 'rho' and 'air' in subscript), which is why I chose rho_air in the code. However in the YAML input we are not limited by variable names and we can use spaces, so 'air rho' seemed more human-readable to me (since 'air' qualifies 'rho', as a substitute for 'the rho of air', which English language would reduce to 'air rho'). But it can be changed if you think it is more consistent!