Refactor in to library - part I #64

Flaminietta · 2025-01-28T13:59:29Z

I finally came to have a good working basis of ProMCDA refactored into a library. In this first part, you can find the following major improvements:

ProMCDA library can be instantiated, and its two main methods can be used independently: normalize and aggregate.
ProMCDA library has only two required input parameters; all others are optional and have default values.
There are no more configuration files or dictionaries defining the input parameters.
Class Enums have been introduced to reduce hard-coded values.
normalize and aggregate implement the sensitivity analysis (full or partial); the robustness analysis (on the indicators or weights); and simple usage with set parameters.
New functions and classes are unit-tested.
A notebook helps to test all functionalities physically.

It's a good moment for you to start reviewing the changes. In particular, I wish that:

@kapil-agnihotri can review the code quality, design, and efficiency.
@mspada can review the overall logic, and the notebook for physical testing and help with the design of end-to-end tests to prove that the new ProMCDA works as expected. Please check a couple of TODOS where I had doubts.

In the meantime, I will continue the refactoring; in particular, we still need to:

Future, optional improvements:

implement a few other methods (e.g., get_plots, save_output)
dynamic method combinations: if the number of methods/combinations (like 13) grows, consider making that dynamic
or stored as an internal attribute so tests remain flexible
improve plot quality - explore plugin option
performance tests
adjust logging as desired

…keys in config settings

…ts, without robustness

…nd scores from dict to dataframe, broken tests

…DTool into refactor_in_to_library

kapil-agnihotri

I am still reviewing the PR but here are some more comments.

promcda/configuration/configuration_validator.py

promcda/utils/utils_for_main.py

promcda/configuration/configuration_validator.py

promcda/models/ProMCDA.py

kapil-agnihotri · 2025-06-04T16:39:40Z

promcda/models/ProMCDA.py

+        validate_configuration(
+            input_matrix=self.input_matrix,
+            polarity=self.polarity,
+            weights=self.weights,


I think same applies to all the parameters. You have already used them in the code but validating them now.

I don't understand, this is one of the first blocks of ProMCDA.py, this validation is happening just before any other processes.

If you refer to the block just before this one, the logic and order of these two blocks are consistent with the needed checks and processing of data.

I'd leave it as it is.

The two previous steps are necessary to validate_configuration.

Example: if the input matrix has a column with no information, this column is dropped in check_input_matrix. The weights and polarities are consequently shortened in process_indicators_and_weights. Only then can we validate the entire configuration settings, for example, estimate the number of indicators in the case of robustness analysis in validate_configuration.

This logic and all the controls could be rewritten more organically and clearly. I leave this improvement to a later version of the library.

Do you think then this is a potential comment for a follow up ticket?

promcda/configuration/configuration_validator.py

promcda/models/ProMCDA.py

kapil-agnihotri

Still in review but here are some more comments.

promcda/models/mcda_without_robustness.py

promcda/utils/utils_for_main.py

promcda/models/ProMCDA.py

promcda/models/mcda_without_robustness.py

promcda/models/ProMCDA.py

kapil-agnihotri

I am finished with the review.

kapil-agnihotri · 2025-06-06T12:25:56Z

promcda/models/ProMCDA.py

+                all_weights_score_means_normalized, all_weights_score_stds_normalized = \
+                compute_scores_for_all_random_weights(self.normalized_values_without_robustness, norm_weights,
+                                                      aggregation_method)
+            self.all_weights_score_means = all_weights_score_means


No need to create local variable just to reassign them to member variable.

Assigning these variables to self. is intentional because it makes them accessible throughout the class instance, beyond the scope of the current method. Without this, the variables would be local and not available to other methods or after initialization. So this assignment is necessary for maintaining the state within the class.

I mentioned about the local variables and not self variables. What I meant you can do is:

self.all_weights_score_means, self.all_weights_score_stds, \ self.all_weights_score_means_normalized, self.all_weights_score_stds_normalized = \ compute_scores_for_all_random_weights(self.normalized_values_without_robustness, norm_weights, aggregation_method)

Looks like this is still pending :(. Please assign self.___ variables directly instead of using intermediate variable.

promcda/models/ProMCDA.py

promcda/utils/utils_for_parallelization.py

promcda/utils/utils_for_main.py

promcda/models/ProMCDA.py

promcda/utils/utils_for_main.py

promcda/utils/utils_for_parallelization.py

…DTool into refactor_in_to_library

kapil-agnihotri

I see that a lot of comments were unaddressed (most of them were from collapsable messages). Can you please address or reject them however you feel them fit? Few of them also corresponds to some logic.
Some comments were resolved without any changes so I have unresolved them.
I have also added some comments on your new logic.

README.md

promcda/configuration/configuration_validator.py

promcda/models/ProMCDA.py

kapil-agnihotri · 2025-06-21T10:38:08Z

promcda/models/ProMCDA.py

+        if not isinstance(robustness, RobustnessAnalysisType):
+            raise TypeError(f"'robustness' must be of type RobustnessAnalysisType, got {type(robustness).__name__}")
+
+        if self.weights is None and RobustnessAnalysisType.INDICATORS.value is not "indicators":


I don't know if I missed this before but I doubt that condition RobustnessAnalysisType.INDICATORS.value is not "indicators" would ever fail.
You are comparing enum value to a string, this would always validate to false, i.e. indicators != indicators.

promcda/models/ProMCDA.py

+        validate_configuration(
+            input_matrix=self.input_matrix,
+            polarity=self.polarity,
+            weights=self.weights,


kapil-agnihotri · 2025-06-21T11:51:25Z

promcda/models/ProMCDA.py

        # Apply aggregation in the different configuration settings
        # NO UNCERTAINTY ON INDICATORS AND WEIGHTS
-        if not self.robustness_indicators and not self.robustness_weights and not self.robustness_single_weights:
+        if (not self.robustness == RobustnessAnalysisType.INDICATORS


Why not simply check for None instead of checking for != for all the other values of enum? If I understand correctly then here you are not checking for individual values but just for a single value?

Any update here? Looks like this was missed.

kapil-agnihotri · 2025-06-21T11:52:59Z

promcda/models/ProMCDA.py


        # NO UNCERTAINTY ON INDICATORS, ALL RANDOMLY SAMPLED WEIGHTS (MCDA runs num_samples times)
-        elif self.robustness_weights and not self.robustness_single_weights and not self.robustness_indicators:
+        elif (self.robustness == RobustnessAnalysisType.ALL_WEIGHTS


Can self.robustness have 3 different values at a same time? If not then why do you check for all the values? i would just check for self.robustness == RobustnessAnalysisType.ALL_WEIGHTS.

Any update here?

kapil-agnihotri · 2025-06-21T11:53:19Z

promcda/models/ProMCDA.py


        # NO UNCERTAINTY ON INDICATORS, ONE SINGLE RANDOM WEIGHT AT TIME
-        elif self.robustness_single_weights and not self.robustness_weights and not self.robustness_indicators:
+        elif (self.robustness == RobustnessAnalysisType.SINGLE_WEIGHTS


Same as before Can self.robustness have 3 different values at a same time?

kapil-agnihotri · 2025-06-21T11:53:29Z

promcda/models/ProMCDA.py


        # UNCERTAINTY ON INDICATORS, NO UNCERTAINTY ON WEIGHTS
-        elif self.robustness_indicators and not self.robustness_weights and not self.robustness_single_weights:
+        elif (self.robustness == RobustnessAnalysisType.INDICATORS


Same as before Can self.robustness have 3 different values at a same time?

kapil-agnihotri · 2025-06-21T11:53:56Z

promcda/models/ProMCDA.py

        print(f"MCDA ranks are retrieved")

-        if not any([self.robustness_weights, self.robustness_indicators, self.robustness_single_weights]):
+        if not any([self.robustness == RobustnessAnalysisType.ALL_WEIGHTS,


Just check for None instead of all 3?

kapil-agnihotri · 2025-06-25T07:52:49Z

README.md

+Together, these examples show both the mechanics of using the library and the applicability of its concepts.
+
+In particular, `demo_in_notebook` contains:
+- Two examples of setups for instatiating the ProMCDA object: one with a dataset without uncertainties and one with a dataset with uncertainties.


I can see only python notebook in demo_in_notebook folder. But you mentioned that there are two examples setups contained in demo_in_notebook.
If they are created within notebook then it would be great mention it here because you are referring to the folder instead of notebook. Otherwise someone might search for input files in the folder.

They are created in the notebook, at the beginning. I edit the README.

kapil-agnihotri · 2025-06-25T08:05:33Z

promcda/models/ProMCDA.py

                 weights: Optional[list] = None,
-                 robustness_weights: Optional[bool] = False,
-                 robustness_single_weights: Optional[bool] = False, robustness_indicators: Optional[bool] = False,
+                 robustness: RobustnessAnalysisType = RobustnessAnalysisType.NONE,


Thank you for the change to enum. It would also be great to update docs for this method as they still refer to old implementation with combination of robustness and weights.

kapil-agnihotri · 2025-06-25T08:06:31Z

promcda/models/ProMCDA.py

+        - The input_matrix should contain the alternatives as rows and the criteria as columns.
+        - If weights are not provided, they are set to 0.5 for each criterion.
+        - If robustness_weights is enabled, the robustness_single_weights should be disabled, and viceversa.
+        - If robustness_indicators is enabled, the robustness on weights should be disabled.


Thanks a lot for the change. Can you please also update docs for this method.

kapil-agnihotri · 2025-06-25T08:14:46Z

promcda/models/ProMCDA.py

+        if not isinstance(robustness, RobustnessAnalysisType):
+            raise TypeError(f"'robustness' must be of type RobustnessAnalysisType, got {type(robustness).__name__}")
+
+        if self.weights is None and RobustnessAnalysisType.INDICATORS.value != "indicators":


I think this logic still needs to be corrected. Let me explain what you are doing here. You are trying to compare enum with a string.
Your enum RobustnessAnalysisType.INDICATORS.value will give you value indicators and this is compared with negation to indicators. i.e., indicators != indicators.
What you might want to do here could be robustness.value != indicators.

You were probably referring to a previous commit here. The check here is already of the kind: self.robustness != RobustnessAnalysisType.INDICATORS, that is correct and tested.

kapil-agnihotri · 2025-06-25T08:16:35Z

promcda/models/ProMCDA.py

+        validate_configuration(
+            input_matrix=self.input_matrix,
+            polarity=self.polarity,
+            weights=self.weights,


Do you think then this is a potential comment for a follow up ticket?

kapil-agnihotri · 2025-06-25T08:30:01Z

promcda/models/ProMCDA.py

        # Apply aggregation in the different configuration settings
        # NO UNCERTAINTY ON INDICATORS AND WEIGHTS
-        if not self.robustness_indicators and not self.robustness_weights and not self.robustness_single_weights:
+        if (not self.robustness == RobustnessAnalysisType.INDICATORS


Any update here? Looks like this was missed.

kapil-agnihotri · 2025-06-25T08:30:48Z

promcda/models/ProMCDA.py


        # NO UNCERTAINTY ON INDICATORS, ALL RANDOMLY SAMPLED WEIGHTS (MCDA runs num_samples times)
-        elif self.robustness_weights and not self.robustness_single_weights and not self.robustness_indicators:
+        elif (self.robustness == RobustnessAnalysisType.ALL_WEIGHTS


Any update here?

kapil-agnihotri · 2025-06-25T08:33:42Z

promcda/models/ProMCDA.py

+                all_weights_score_means_normalized, all_weights_score_stds_normalized = \
+                compute_scores_for_all_random_weights(self.normalized_values_without_robustness, norm_weights,
+                                                      aggregation_method)
+            self.all_weights_score_means = all_weights_score_means


Looks like this is still pending :(. Please assign self.___ variables directly instead of using intermediate variable.

kapil-agnihotri · 2025-06-25T08:34:02Z

promcda/models/ProMCDA.py


        # NO UNCERTAINTY ON INDICATORS, ONE SINGLE RANDOM WEIGHT AT TIME
-        elif self.robustness_single_weights and not self.robustness_weights and not self.robustness_indicators:
+        elif (self.robustness == RobustnessAnalysisType.SINGLE_WEIGHTS


kapil-agnihotri · 2025-06-25T08:48:07Z

promcda/configuration/configuration_validator.py

+            return polarity, norm_fixed_weights, None, None
+            #  Return None for norm_random_weights and rand_weight_per_indicator
+        else:
+            output_weights = handle_robustness_weights(mc_runs, num_indicators, robustness_weights,


Yes, if you want then you can further improve it as (as per my initial suggestion)

norm_random_weights, rand_weight_per_indicator = handle_robustness_weights(mc_runs, num_indicators, robustness)

Flaminietta · 2025-07-01T11:13:20Z

@kapil-agnihotri Please approve.

Flaminietta added 30 commits October 8, 2024 19:14

refactor:start adding new logic for library

eae5fec

refactor: add unit test for ProMCDA.py

61bf9a4

refactor: solve some unit tests, circual imports

993a211

unit test debugging 1

c8180db

refactor: add basic uni tests and remove open socket warning

7742a1a

refactor: delete config.py and add a function to validate dictionary …

9d747ca

…keys in config settings

refactor: add normalization as a method, failing tests

88e31c5

refactor: add enums for enumerations

d9e03b1

refactor: simplify normalize and aggregate methods and draft unit tes…

5c0cd1d

…ts, without robustness

add notebook for testing library function, change normalized_scores a…

1378354

…nd scores from dict to dataframe, broken tests

stop tracking files in demo_in_notebook/output_files/mock_output/

c274e4b

update gitignore

5344494

fix test_aggregate_with_specific_normalization_and_aggregation_methods

d980127

fix all test_promcda

3852c11

test in notebook basic methods normlize and aggregate

8ca4f1d

improve column names in normalize

d312433

change and simplify the __init__, add optional and default parameters

7deee2d

modify normalize method and test for mcda without robustness

f7efe84

add normalization with robustness

fbf90ee

modify test for norm with robustness

93441d2

add and test normalization with robustness on indicators

a064fc0

finalize normalization

a770cc1

add aggregation with specific method

d587fe3

add aggregation without a specific method

d95aaa9

add aggregation with robustness

20cacda

fix unit test for aggregation, all variations of it

39c9061

update notebook with end to end tests

b9272a0

fixing aggregation with robustness

1d71997

fix aggregation with robustness on indicators and weights

17596e6

remve hard coded column names and use enums

eed0195

Flaminietta added 3 commits June 4, 2025 12:01

delete pycache

caa73d3

Merge branch 'refactor_in_to_library' of github.com:wetransform-os/MC…

62dd726

…DTool into refactor_in_to_library

Update README.md

784d799

kapil-agnihotri requested changes Jun 4, 2025

View reviewed changes

kapil-agnihotri reviewed Jun 5, 2025

View reviewed changes

kapil-agnihotri requested changes Jun 6, 2025

View reviewed changes

Flaminietta added 11 commits June 12, 2025 15:13

bugfix: num of indicators calculated correctly in case of robustness

c207457

Merge branch 'refactor_in_to_library' of github.com:wetransform-os/MC…

44b038e

…DTool into refactor_in_to_library

bugfix: fix calculation of PFD masks

c3139ae

bugfix: fix distribution type name

6ca91c8

Bugfix: correct count of indicators in case of robustness

aa3aee7

update the README

6927f98

remove toy example

be2163d

fix utils_for_main

c6d9cf2

fix naming of input_matrix_indexed_by_alternatives

ff32f9f

address PR's comments and fix tests

2379175

fix demo

110bac4

kapil-agnihotri requested changes Jun 21, 2025

View reviewed changes

Flaminietta added 4 commits June 22, 2025 20:05

remove hard paths from demo

39e8067

improve code quality and fix tests

6b5fc05

delete utils_for_plotting, fix unseen comments, fix tests

5b75231

debug after training

397c57e

kapil-agnihotri reviewed Jun 25, 2025

View reviewed changes

Flaminietta added 3 commits June 26, 2025 17:48

add training, address last comments, bug in standardization not solved

073947c

fix bug in standardization and add training

c746593

update changelog

260a506

kapil-agnihotri approved these changes Jul 2, 2025

View reviewed changes

stempler merged commit 056fa10 into main Jul 2, 2025
2 checks passed

stempler deleted the refactor_in_to_library branch July 2, 2025 16:06

Refactor in to library - part I #64

Refactor in to library - part I #64

Uh oh!

Conversation

Flaminietta commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kapil-agnihotri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Flaminietta Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kapil-agnihotri left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kapil-agnihotri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kapil-agnihotri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kapil-agnihotri Jun 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Flaminietta commented Jan 28, 2025 •

edited

Loading

Flaminietta Jun 18, 2025 •

edited

Loading

kapil-agnihotri left a comment •

edited

Loading

kapil-agnihotri Jun 21, 2025 •

edited

Loading