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

kapil-agnihotri · 2024-09-04T14:18:43Z

…ntation to use model classes

github-actions

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

…ntation to use model classes

kapil-agnihotri · 2024-09-10T14:27:05Z

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 · 2024-09-18T14:07:07Z

@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 · 2024-09-23T06:59:09Z

@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 · 2024-09-24T09:21:22Z

@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 · 2024-09-24T09:59:45Z

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 · 2024-09-19T07:47:13Z

input_files/toy_example/car-data-from-ranking-service-robustness-on-weights-no-sensitivity.json

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"?

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 · 2024-09-19T08:27:31Z

mcda/mcda_run.py

@@ -7,21 +7,21 @@
 Usage (from root directory):


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

Flaminietta · 2024-09-19T08:30:29Z

mcda/mcda_run.py


+def config_dict_to_configuration_model(input_config):


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

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 """

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

Flaminietta · 2024-09-19T08:40:02Z

mcda/mcda_run.py


+def config_dict_to_configuration_model(input_config):
    # Extracting relevant configuration values
    config = Config(input_config)
    input_matrix = read_matrix(config.input_matrix_path)


Retrieving the input_matrix_path is possible from the old version of the configuration file (configuration.json). But it's not valid anymore with the new kind of configuration that you created in input_files/toy_example. What is your plan?

Flaminietta · 2024-09-19T08:56:16Z

mcda/mcda_ranking_run.py

+
+
+# noinspection PyTypeChecker
+def main_using_model(input_config: dict) -> dict:


Minor: I am unsure if main_using_model is the best function name. What do you think of main_using_config? Is the config that makes a difference, the model is always the same (i.e., MCDA).

Flaminietta · 2024-09-24T08:47:23Z

mcda/models/base_model_.py

+T = typing.TypeVar('T')
+
+
+class Model(object):


Please add a docstring and in summary explain something like:
The class Model is designed to represent a data model with serialization and deserialization capabilities. It provides a framework for representing data models that can be easily serialized to and from dictionaries. It supports operations for equality checks, and for creating more complex models in Python applications with data exchange.

Flaminietta · 2024-09-24T08:48:55Z

mcda/models/configuration.py

+from ProMCDA.mcda import util
+
+
+class Configuration(Model):


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

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 · 2024-09-24T08:49:49Z

mcda/models/configuration.py

+
+    @property
+    def sensitivity(self) -> ConfigurationSensitivity:
+        """Gets the sensitivity of this Configuration.


Please adjust the docstrings to be less misleading.

Flaminietta · 2024-09-24T08:59:23Z

mcda/models/configuration_modified_input.py

I see that these are all automatically generated scripts, and probably all necessary then. However, for me, the overall package structure became quite confusing. The name of this script is also confusing: why "modified"?

I am not sure we can make something different.

Do I understand well that, if one day we want to modify something in the original configuration settings (e.g., by adding a new functionality), then all of these automatically created scripts won't be manually changed but Swagger will recreate them?

Flaminietta · 2024-09-24T09:11:48Z

tests/unit_tests/test_mcda_with_robustness.py

@@ -100,7 +104,7 @@ def get_list_random_input_matrices() -> list[pd.DataFrame]:
        config = TestMCDA_with_robustness.get_test_config()
        config = Config(config)
        mcda_with_robustness = MCDAWithRobustness(config, input_matrix, is_exact_pdf_mask, is_poisson_pdf_mask)
-        output_list = mcda_with_robustness.convert_list(out_list)
+        output_list = MCDAWithRobustness.convert_list(out_list)


Weird, you instantiated MCDAWithRobustness in an object, why do you then use MCDAWithRobustness.convert_list(out_list)? I believe that it works if you made it...

github-actions bot reviewed Sep 4, 2024

View reviewed changes

kapil-agnihotri force-pushed the refactor-use-model-classes-and-also-refactor-original-config-implementation-to-use-model-classes branch 2 times, most recently from a79aa2f to 1acb08a Compare September 9, 2024 15:41

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

5790d7d

…ntation to use model classes

kapil-agnihotri force-pushed the refactor-use-model-classes-and-also-refactor-original-config-implementation-to-use-model-classes branch from 1acb08a to 5790d7d Compare September 9, 2024 16:03

kapil-agnihotri marked this pull request as ready for review September 9, 2024 16:03

kapil-agnihotri requested review from thorsten-reitz and Flaminietta September 9, 2024 16:03

kapil-agnihotri added 3 commits September 10, 2024 11:10

fix: failing tests

943f2c6

refactor: refer ProMCDA models instead of ranking-service models

a809166

fix: tests to import correct Configuration object

02a26a5

kapil-agnihotri force-pushed the refactor-use-model-classes-and-also-refactor-original-config-implementation-to-use-model-classes branch from c75409e to 02a26a5 Compare September 10, 2024 14:22

Flaminietta requested a review from mspada September 18, 2024 14:03

thorsten-reitz approved these changes Sep 24, 2024

View reviewed changes

Flaminietta requested changes Sep 24, 2024

View reviewed changes

refactor: update to run ProMCDA from cli

a5f92f9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kapil-agnihotri commented Sep 4, 2024

github-actions bot left a comment

kapil-agnihotri commented Sep 10, 2024

Flaminietta commented Sep 18, 2024 •

edited

Loading

kapil-agnihotri commented Sep 23, 2024

Flaminietta commented Sep 24, 2024

thorsten-reitz left a comment

Flaminietta commented Sep 24, 2024

Flaminietta Sep 19, 2024

Flaminietta Sep 19, 2024

Flaminietta Sep 19, 2024

Flaminietta Sep 19, 2024

Flaminietta Sep 19, 2024

Flaminietta Sep 19, 2024

Flaminietta Sep 19, 2024

Flaminietta Sep 19, 2024

Flaminietta Sep 24, 2024

Flaminietta Sep 24, 2024

Flaminietta Sep 24, 2024

Flaminietta Sep 24, 2024

Flaminietta Sep 24, 2024

Flaminietta Sep 24, 2024

Flaminietta Sep 24, 2024



		# noinspection PyTypeChecker
		def main_using_model(input_config: dict) -> dict:

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

Are you sure you want to change the base?

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

Conversation

kapil-agnihotri commented Sep 4, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

kapil-agnihotri commented Sep 10, 2024

Flaminietta commented Sep 18, 2024 • edited Loading

kapil-agnihotri commented Sep 23, 2024

Flaminietta commented Sep 24, 2024

thorsten-reitz left a comment

Choose a reason for hiding this comment

Flaminietta commented Sep 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Flaminietta commented Sep 18, 2024 •

edited

Loading