refactor: use model classes and also refactor original config impleme…

[kapil-agnihotri]

…ntation to use model classes

[github-actions]

Hello @kapil-agnihotri, thank you for submitting a pull request. We will address it.

[kapil-agnihotri]

For now I have duplicated all model classes in here. I think later ranking-service can be modified to refer to the model classes from ProMCDA submodule. This will also reduce code duplication.
The only drawback would be to modify imports when there would be some change to model classes.

[Flaminietta]

@kapil-agnihotri
I suggest creating a branch named develop (or similar) and using it to merge the PRs that will lead to a refactored version of the package.

The main reasons are:

• the attached paper in publication in JOSS that refers to the current version of the tool in main;
• the software in main is thoroughly tested with some test cases (end-to-end controlled tests);
• the current users of the package can keep referring to the working and tested version in main, and to its readme, while we accomplish a complete refactoring

[kapil-agnihotri]

@Flaminietta thank you for your feedback. I can create a new branch and push the changes to it once the review is complete. I believe that the review is still in progress?

[Flaminietta]

@kapil-agnihotri @mspada
I went through the entire PR. I am quite overwhelmed by the many changes. I would propose that the two or three (with @mspada) of us discuss together once briefly the new structure proposed. I believe that you @kapil-agnihotri can
explain to me/us the new needs and give an explanatory overview of all the changes.

Did you already perform also end-to-end controlled tests?

I still need to run it once. Do I run it from the terminal as before?

[thorsten-reitz]

Though I didn't get around to running it, the principal structure is as I would have expected.

[Flaminietta]

The README needs to be updated too for the new refactoring.

For example, now the run is not initiated by typing:
python3 -m mcda.mcda_run -c configuration.json but by typing
python3 -m mcda.mcda_run -c python3 -m mcda.mcda_run -c car-data-from-ranking-service-robustness-on-weights-no-sensitivity.json, right?

However, for now I cannot run it, I should dig deeper to find the issue.

[Flaminietta]

I understand that this file represents a combination of the previous configuration.json. file and the input_matrix.cvs. I would name it referring to the idea of a configuration anyway, like "configuration_car_data_rank_robustness_on_weight_no_sensitivity.json"

In "robustness", what happens if the user selects "robustness": "indicators"? The entry "onWeightsLevel" will be then substituted with "on_indicators"?

[Flaminietta]

Additionally, in this example writing the input matrix is easy (even manually). What would the strategy be when the input matrix is a table with many more rows and columns?

[Flaminietta]

I guess that we won't run it from CLI anymore.

[kapil-agnihotri]

It will, I have updated the commands.

[Flaminietta]

I would move the function somewhere outside of this script. It's a utility, let's keep this running script clear.

[Flaminietta]

Please add a docstring to the function by following the format used for the other functions.
Something like:

"""
    Describe functionality.

    Parameters:
    - A (type): description.
    - B (type): description.

    Raises:
    - ValueError: when it happens.

    :param A: type
    :param B: type
    :return R: type
    """

[Flaminietta]

Please add the type hinting (that indicates the expected type of the value that the function will return).

[kapil-agnihotri]

Done. Please let me know if you still feel something is missing.

[Flaminietta]

def config_dict_to_configuration_model(input_config) -> HINT TYPE OF RETURN:
"""
    Method that converts Config object into a ConfigurationModel.
    This method is used when ProMCDA is called using cli where input matrix is passed
    in a separate file.

    Parameters:
        - input_config: EXPLAIN HERE WHAT OBJECT YOU EXPECT (SHORT DESCRIPTION)
    Returns: 
         - EXPLAIN HERE WHAT OBJECT YOU EXPECT (SHORT DESCRIPTION)
        
    :param input_config: dict
    :rtype: dict - ALREADY GIVEN ABOVE, NOT NECESSARY
    :return: GIVE THE TYPE OF RETURN
    """

[Flaminietta]

I have tried to respect this format throughout the entire package.

[Flaminietta]

If the original class Config stays too, we need perhaps to review naming. It's quite confusing for me.

[Flaminietta]

Additionally, adding short descriptions (docstrings) of the main functionalities helps navigate the various scripts.
For example:
The Configuration class encapsulates a configuration for MCDA, allowing for structured data representation and easy manipulation of attributes.

[Flaminietta]

@kapil-agnihotri after my second review, this is my feedback:

• I can successfully run both the package and the ranking_service with the following commands:
  python3 -m ProMCDA.mcda.mcda_ranking_run -c ProMCDA/configuration.json
python3 -m ProMCDA.mcda.mcda_ranking_run -c ProMCDA/input_files/toy_example/car-data-input-sample-from-ranking-service.json
• Please move mcda/util.py also into mcda/utils/; can we rename it somehow more explicitly?
• Please clarify the coexistence of mcda_run.py and mcda_ranking_run.py: why are they both needed? What is the difference in their functionality? Are they both entry points? Use file names that help to understand their functionality and differences.
• I feel that configuration/config.py and models/configuration.py need to be better distinguished too: what are their functionality and differences? Consider renaming it if necessary.
• Please try to align the format of the docstrings (I have left a couple of comments above)
• Unit tests pass, do we need to rerun end-to-end tests?
• Do we need to add extra unit tests?
• For completeness, the API written in Swagger to call the ranking_service shouldn't also be saved in this repository?

About the end-to-end tests:

• I have a reference example called forest_example (not in GitHub) used in the past to test the correctness of calculations.
• We can use the same reference.
• We need therefore to create 11 configuration models similarly as you have done for the toy example; these 11 configurations represent all the possible cases for running ProMCDA (see screenshot).
• How do we fill in the input matrix dictionary needed in those configuration files? This is a general question: how does one do it automatically when the input matrix is big?

We can have a call any time.

[kapil-agnihotri]

Please clarify the coexistence of mcda_run.py and mcda_ranking_run.py: why are they both needed?

Yes, mcda_run created by you for cli that accepts config.json in old format.
mcda_ranking_run.py for ranking service API and can also be used from cli with new configuration.json structure that ranking service passes to ProMCDA. I could have merged the code into single class but kept them in separate classes. In my opinion class names are ok. Please feel free to suggest if you don't feel that way.

I feel that configuration/config.py and models/configuration.py need to be better distinguished too: what are their functionality and differences? Consider renaming it if necessary.

One (config.json) works on plain json input and python primitive data types and the other (models/configuration.json) works on models and classes structure. I cannot rename classes from models/... as they are generated from swagger and are used by connexions library to convert json into python models. So if you want we can rename config.py. However, with your new implementation of converting it into library I think you want to get rid of configuration/config.py.

Unit tests pass, do we need to rerun end-to-end tests?

Sure, if you want to. I am not sure how to run them.

Do we need to add extra unit tests?

I think if all the existing unit test passes then we don't need anything extra. However, if you feel then we can for sure add extra tests if there is scope.

For completeness, the API written in Swagger to call the ranking_service shouldn't also be saved in this repository?

No it is the other way round. ProMCDA is used as a submodule/ (library in future) in ranking-service. Ranking-service is the parent service which calls a module(ProMCDA) to get the job done.

How do we fill in the input matrix dictionary needed in those configuration files? This is a general question: how does one do it automatically when the input matrix is big?

Not sure if you already looked at my PR of ranking service but there spent some time to create another API that converts huge input csv matrix to desired input_matrix in json format to be used in ranking service.

Thank you for the feedback Flaminia, I think I should pause my work in this branch for the beautification on documentation you proposed. As we discussed now you want to convert ProMCDA into library by getting rid of config/Configuration json/object. I think if the ProMCDA is again refactored then I would unnecessarily be spending time in beautifying the code here that might not be used in the future.