Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Start adding new logic for library #53

Draft
wants to merge 30 commits into
base: develop
Choose a base branch
from

Conversation

Flaminietta
Copy link
Collaborator

ProMCDA is refactored to become a library initiated in a Python environment by a few input parameters. There is no need for a configuration file, and intermediary and final calculations are exposed to the user.

ProMCDA is also refactored to be modular and flexible.

The refactoring facilitates the use of an API to call ProMCDA as a service.

@Flaminietta Flaminietta added the enhancement New feature or request label Oct 8, 2024
@Flaminietta Flaminietta self-assigned this Oct 8, 2024
@Flaminietta Flaminietta force-pushed the refactor_in_to_library branch from cef2ab3 to eae5fec Compare October 8, 2024 17:17
@Flaminietta Flaminietta force-pushed the refactor_in_to_library branch from d232c1d to 88e31c5 Compare October 22, 2024 19:49
@Flaminietta
Copy link
Collaborator Author

Flaminietta commented Nov 12, 2024

@kapil-agnihotri, as discussed today, you can start having a look at the progress of transforming ProMCDA into a library and leave me any comments and suggestions. @mspada, you can also start familiarising yourself with the transformation of ProMCDA.

This is not a working version yet. From the commit where I changed the output format of the normalised criteria values and scores to DataFrames (instead of dictionaries) to expose them directly to the user calling the "normalize" and "aggregate" methods, the tests fail and you cannot use the library in the notebook. I am currently working on this.

My plan for the next release is to (in no particular order)

  • fix the tests and be able to instantiate ProMCDA and use its main methods (i.e.,normalize, aggregate, run, get_results);

  • clean up redundant code;

  • simplify the init of ProMCDA.py for the user who only wants to run the "normalize" and "aggregate" methods by making some parameters optional;

  • give the option to normalize either with zero values or only with values greater than zero;

  • give the option of outputting DataFrames or dictionaries (given my recent experience, this may not be that easy);

  • make logging optional/clean where needed;

  • eliminate input dictionaries and implement classes instead;

  • simplify the configuration logic and terminology (see this branch).

In the final version, all tests should run again and we need to proof all the functionalities with test data.

MINIMUM = 'minimum'


class NormalizationNames4Sensitivity(Enum):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would refactor this enum to SensitivityNormalization

RANK = 'rank'


class OutputColumnNames4Sensitivity(Enum):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would refactor it to SensitivityOutputColumns

"""
Names of output columns in case of sensitivity analysis
"""
WS_MINMAX_01 = 'ws-minmax_01'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applies to all the enums from L47 to L59:
Why do they have hyphen- and underscore _ both in values?
I would recommend to use underscore _ for all the values as it would increase readability and also will be as per the standards.
Although they are just output column names, still it would be great to avoid hyphen.

:rtype: bool
"""

keys_of_dict_values = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great to simplify these keys based on ranking service logic and open API specification that we created in the past.
robustness
monte_carlo

Also rename keys based on Open API specification.


keys_of_dict_values = {
'sensitivity': ['sensitivity_on', 'normalization', 'aggregation'],
'robustness': ['robustness_on', 'on_single_weights', 'on_all_weights', 'given_weights', 'on_indicators'],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, it would be preferable to add these keys in enum instead of defining them here otherwise there would be multiple occurrence of these spread in your code.

Tuple[List[str], None, None, dict]]:
"""
Manage polarities and weights based on the specified robustness settings, ensuring that the appropriate adjustments
and normalizations are applied before returning the necessary data structures.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any doc on return types?

rand_weight_per_indicator = {}

# Managing polarities
if is_robustness_indicators == 0:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again no need for explicit comparison to 0 or 1

self.aggregated_matrix = None
self.ranked_matrix = None

def validate_inputs(self) -> Tuple[int, int, list, Union[list, List[list], dict], dict]:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect this method to only validate and not to extract values

def run_mcda(self, is_robustness_indicators: int, is_robustness_weights: int,
weights: Union[list, List[list], dict]):
"""
Execute the full ProMCDA process, either with or without uncertainties on the indicators.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

incomplete docs

if method is None or method == NormalizationFunctions.TARGET.value:
indicators_target_01 = norm.target(feature_range=(0, 1))
indicators_target_without_zero = norm.target(feature_range=(0.1, 1))
add_normalized_df(indicators_target_01, "target_01")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hard coded string you can easily use enums here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants