fast-aircraft-design / FAST-OAD

FAST-OAD: An open source framework for rapid Overall Aircraft Design
GNU General Public License v3.0
47 stars 25 forks source link

Issue 440 reset null submodels #444

Closed christophe-david closed 2 years ago

christophe-david commented 2 years ago

Solves #440.

This PR ensures that all sub-models are reactivated before processing sub-models from the configuration file.

Some modifications have also been done to the documentation.

codecov[bot] commented 2 years ago

Codecov Report

Merging #444 (ae8a74c) into master (a9e8826) will increase coverage by 0.01%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #444      +/-   ##
==========================================
+ Coverage   89.47%   89.48%   +0.01%     
==========================================
  Files          73       73              
  Lines        4416     4422       +6     
  Branches      709      711       +2     
==========================================
+ Hits         3951     3957       +6     
  Misses        337      337              
  Partials      128      128              
Impacted Files Coverage Δ
src/fastoad/io/configuration/configuration.py 91.13% <100.00%> (+0.03%) :arrow_up:
src/fastoad/module_management/service_registry.py 100.00% <100.00%> (ø)

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 a9e8826...ae8a74c. Read the comment docs.

christophe-david commented 2 years ago

First of all thanks for the PR, I've tried it on the test code that was sent with the original issue and it works !

This has however raised a question for me :

* Let's assume that we had, in the file `dummy_submodels\submodel.py`, these two lines:
RegisterSubmodel.active_models["requirement.1"] = "req.1.submodel"
RegisterSubmodel.active_models["requirement.2"] = "req.2.submodel.A"

The same error as in the test will happen. I assumed, before re-reading the readthedocs that, since the second line was there, after the deactivation and deactivation cancel req.2.submodel.A would be used, which is not the case. It is mentioned in the readthedocs that the last line preempts and that the configuration file is the right way to do it, but maybe this case should be highlighted, or at least the fact that deactivating a submodel counts as a choice of submodel selection.

One thing to be noted, is that the change I made focuses on cancelling previous submodel deactivations only, not the settings where a submodel ID is provided.

A more definitive solution would be to reset completely RegisterSubmodel.active_models when reading a configuration file, but that would render useless any attempt to set the submodels using Python, since these instructions would finally not been taken into account.

So, yes, it may lead to unwanted behavior. For instance, let's assume that:

I don't know if it is worth fixing such corner case, but if we want to, I think the only solution would be to separate settings that come from Python and those from configuration files, to be able to reset only configuration file settings. More drastically, we could just disallow the Python way. @ScottDelbecq, @esnguyenvan, perhaps you will also have an opinion about that ?

Anyway, I modified the documentation so that it promotes the usage of configuration file over Python for the submodel setting.

christophe-david commented 2 years ago

(dependeing on the fact that there might be two submodels for the same service in the base distribution of FAST-OAD, which is the case in FAST-OAD_CS23 😁).

In such case, I would strongly recommend that the module provides an option that allows to select the submodel. That does not prevent to use the submodel mechanism internally, but I would really want users to be confronted as few as possible to this feature, that I prefer to consider as a last resort. When I call this feature "experimental", I mean it. It may change significantly in the future, especially if you want features like the one below.

Also, wouldn't it be a good idea to have a way to list services and associated submodels, like list_modules for modules ?

I would be good, but it would require some rework. The list of available submodels should be obtained rather easily, but currently, the services are "created" by the Python line where they are asked for. So, the only way to know them is currently to run all the Python code.