DARMA-tasking / LB-analysis-framework

Analysis framework for exploring, testing, and comparing load balancing strategies
Other
3 stars 1 forks source link

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

Closed ppebay closed 1 year ago

ppebay commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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