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

Factor out useful features of rank_object_enumerator.py and delete the rest #386

Closed
ppebay opened this issue May 11, 2023 · 4 comments · Fixed by #398
Closed

Factor out useful features of rank_object_enumerator.py and delete the rest #386

ppebay opened this issue May 11, 2023 · 4 comments · Fixed by #398
Assignees
Labels
enhancement New feature or request

Comments

@ppebay
Copy link
Contributor

ppebay commented May 11, 2023

rank_object_enumerator.py no longer works (nor is tested) in its current form, as reported by @pierrepebay as part of the review of PR #374

However one of its routines is needed by LBAF_app.py; and this routine itself calls functions of rank_object_enumerator.py, and further.

The goal of this issue is therefore to extract what is useful in rank_object_enumerator.py into a helper class (e.g. lbsStatistics) and then deprecate/remove the rest from the code base.

@tlamonthezie
Copy link
Contributor

In fact the routine is used by the BruteForceAlgorithm.
After some refactoring as requested, I encounter an issue :
By running a BruteFormceAlgorithm we are facing the following error AttributeError: 'AffineCombinationWorkModel' object has no attribute 'aggregate'.
It seems to be an issue with a missing function that has been removed 5 months ago from the WorkModel classes.
@ppebay After having processed this issue should we need to re-open a new issue ? or is it better to process now as a part of this issue ?

@ppebay
Copy link
Contributor Author

ppebay commented Jun 5, 2023

Thanks @tlamonthezie the main issue is that the brute force algorithm is currently calling a stand-alone script that I wrote a while back to check the LBAF optima (but at the time it was not part of LBAF). So it has a much more lightweight data model, but it had to be made to fit into the broader LBAF data model in order to be implemented as a concrete subclass of the abstract base class of LBAF balancing algorithms.

That's why I created that static aggregate method, which was a way to get around the absence of a work model in the aforementioned script. But it's basically just that: a method to get around. It must be deprecated.

Instead, what should be done is the following: properly reimplement the logic of this standalone script inside the lbsBruteForceAlgorithm class, and as a result be able to do away with the aggregate bypass.

I am happy to do that, but can you please push your current work on this issue in a new, draft PR? I would then take on from there, and pass you the baton once done.

Please ping me on said PR so I get notified when it's ready to be picked up.

@tlamonthezie
Copy link
Contributor

tlamonthezie commented Jun 5, 2023

Thanks @tlamonthezie the main issue is that the brute force algorithm is currently calling a stand-alone script that I wrote a while back to check the LBAF optima (but at the time it was not part of LBAF). So it has a much more lightweight data model, but it had to be made to fit into the broader LBAF data model in order to be implemented as a concrete subclass of the abstract base class of LBAF balancing algorithms.

That's why I created that static aggregate method, which was a way to get around the absence of a work model in the aforementioned script. But it's basically just that: a method to get around. It must be deprecated.

Instead, what should be done is the following: properly reimplement the logic of this standalone script inside the lbsBruteForceAlgorithm class, and as a result be able to do away with the aggregate bypass.

I am happy to do that, but can you please push your current work on this issue in a new, draft PR? I would then take on from there, and pass you the baton once done.

Please ping me on said PR so I get notified when it's ready to be picked up.

Hello @ppebay I have just created a PR for that
To deprecate the deprecated module I added some message with the logger to tell this is deprecated. In the form do not hesitate to tell me if there is a better way to deprecate a module in LBAF

tlamonthezie added a commit that referenced this issue Jun 6, 2023
tlamonthezie added a commit that referenced this issue Jun 6, 2023
… use loaded `Object` instances instead of dictionaries
tlamonthezie added a commit that referenced this issue Jun 6, 2023
tlamonthezie added a commit that referenced this issue Jun 6, 2023
@tlamonthezie
Copy link
Contributor

tlamonthezie commented Jun 7, 2023

Thanks @tlamonthezie the main issue is that the brute force algorithm is currently calling a stand-alone script that I wrote a while back to check the LBAF optima (but at the time it was not part of LBAF). So it has a much more lightweight data model, but it had to be made to fit into the broader LBAF data model in order to be implemented as a concrete subclass of the abstract base class of LBAF balancing algorithms.
That's why I created that static aggregate method, which was a way to get around the absence of a work model in the aforementioned script. But it's basically just that: a method to get around. It must be deprecated.
Instead, what should be done is the following: properly reimplement the logic of this standalone script inside the lbsBruteForceAlgorithm class, and as a result be able to do away with the aggregate bypass.
I am happy to do that, but can you please push your current work on this issue in a new, draft PR? I would then take on from there, and pass you the baton once done.
Please ping me on said PR so I get notified when it's ready to be picked up.

Hello @ppebay I have just created a PR for that To deprecate the deprecated module I added some message with the logger to tell this is deprecated. In the form do not hesitate to tell me if there is a better way to deprecate a module in LBAF

Hello @ppebay

  1. Some important refactoring has been done due to the fact that brute force optimization was called by both the BruteForceAlgorithm and also by the other algorithms running with brute_force_optimization parameter set to True
    In fact compute_min_max_arrangements_work logic was duplicated and is now shared lbsStatistics (as proposed in this PR) to have the logic in one place
  2. The aggregate function calls have been removed and code has been re-worked
  3. The rank_object_enumerator module is still there because it still contains some functions. BUT these functions are not used except by this script itself. The remaining functions are compute_pairwise_reachable_arrangements, compute_all_reachable_arrangements, recursively_compute_transitions. Even if it is not used do you want we keep it in lbsStatistics ? (This issue says to "extract what is useful" and "then deprecate/remove the rest" but maybe there could be a reason to keep it used<>useful... maybe ?)

I put the PR #398 in review so you can check that work

tlamonthezie added a commit that referenced this issue Jun 7, 2023
tlamonthezie added a commit that referenced this issue Jun 7, 2023
tlamonthezie added a commit that referenced this issue Jun 8, 2023
…ete the rest (#398)

* #386: upgrade rank_object_enumerator script to work

* #386: move used function of rank_object_enumerator to lbsStatistsics module and deprecate rank_object_enumerator

* #386: update LBAFApp to work with functions moved to lbsStatistics

* #386: update some test brute force config file

* #386: fix error in brute BruteForceAlgorithm class

* #386: add some bruteforce run configuration for VS code

* #386: update VSCode launch configuration

* #386: fix problem with refactoring and acceptance tests

* #386: remove n_ranks no more valid attribute

* #386: fix issue in LBAF_App with n_ranks

* #386: fix potential BruteForce invalid phases key

* #386: eliminate duplicated code in BruteForceAlgorithm and LBAF_App + use loaded `Object` instances instead of dictionaries

* #386: simplify method call

* #386: fix bad variable name

* #386: set pylint to fail only on error or with score lower that 8.5

* #386: refactor the BruteForce algorithm to exclude duplicated code

* #386: refactor and remove duplicated code

* #386: improve code quality

* #386: improvement in compute_arrangement_works

* #386: improve some code

* #386: review docstrings with PEP257 in lbaf module and move useful function to lbsStatistics

* #386: fix trailing whitespace
github-actions bot pushed a commit that referenced this issue Jun 8, 2023
…ete the rest (#398)

* #386: upgrade rank_object_enumerator script to work

* #386: move used function of rank_object_enumerator to lbsStatistsics module and deprecate rank_object_enumerator

* #386: update LBAFApp to work with functions moved to lbsStatistics

* #386: update some test brute force config file

* #386: fix error in brute BruteForceAlgorithm class

* #386: add some bruteforce run configuration for VS code

* #386: update VSCode launch configuration

* #386: fix problem with refactoring and acceptance tests

* #386: remove n_ranks no more valid attribute

* #386: fix issue in LBAF_App with n_ranks

* #386: fix potential BruteForce invalid phases key

* #386: eliminate duplicated code in BruteForceAlgorithm and LBAF_App + use loaded `Object` instances instead of dictionaries

* #386: simplify method call

* #386: fix bad variable name

* #386: set pylint to fail only on error or with score lower that 8.5

* #386: refactor the BruteForce algorithm to exclude duplicated code

* #386: refactor and remove duplicated code

* #386: improve code quality

* #386: improvement in compute_arrangement_works

* #386: improve some code

* #386: review docstrings with PEP257 in lbaf module and move useful function to lbsStatistics

* #386: fix trailing whitespace
tlamonthezie added a commit that referenced this issue Jun 8, 2023
tlamonthezie added a commit that referenced this issue Jun 12, 2023
tlamonthezie added a commit that referenced this issue Jun 12, 2023
* #368: move stepper tests and acceptance tests to the unittest tests to include in tox

* #368: update gitignore

* #368: update CI workflow

* #368: update CI workflow

* #368: update gitignore

* #368: rename acceptance to synthetic-acceptance

* #386: refactor

* #368: rename workflow

* #368: fix call error

* #368: fix bug in a test

* #368: fix bad config path for a test

* #368: rename workflow

* #368: upgrade tox and colorama dependency versions

* #368: create a single requirements.txt file

* #368: upgrade colorama version also for lbaf package setup

* #368: refactor and simlify tox.ini

* #368: rename workflow

* #368: remove method duplicate

* #368: pylint fixes

* #368: pylint fixes

* #368: disable some unexpected pylint warnings

* #368: remove unnecessary line

* #368: tentative to display unit tests results better

* #368: keep only one requirements file and try fix Test report action

* #368: try another Test report plugin

* #368: fix issue with test output

* #368: rename tests report

* #368: try again another plugin

* #368: upgrade version of test report plugin

* #368: setoption to report output on CI

* #368: fix remaining quotes and warnings

* #368: fix pylint notice messages
github-actions bot pushed a commit that referenced this issue Jun 12, 2023
* #368: move stepper tests and acceptance tests to the unittest tests to include in tox

* #368: update gitignore

* #368: update CI workflow

* #368: update CI workflow

* #368: update gitignore

* #368: rename acceptance to synthetic-acceptance

* #386: refactor

* #368: rename workflow

* #368: fix call error

* #368: fix bug in a test

* #368: fix bad config path for a test

* #368: rename workflow

* #368: upgrade tox and colorama dependency versions

* #368: create a single requirements.txt file

* #368: upgrade colorama version also for lbaf package setup

* #368: refactor and simlify tox.ini

* #368: rename workflow

* #368: remove method duplicate

* #368: pylint fixes

* #368: pylint fixes

* #368: disable some unexpected pylint warnings

* #368: remove unnecessary line

* #368: tentative to display unit tests results better

* #368: keep only one requirements file and try fix Test report action

* #368: try another Test report plugin

* #368: fix issue with test output

* #368: rename tests report

* #368: try again another plugin

* #368: upgrade version of test report plugin

* #368: setoption to report output on CI

* #368: fix remaining quotes and warnings

* #368: fix pylint notice messages
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
2 participants