-
Notifications
You must be signed in to change notification settings - Fork 268
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
Generic implementation of optimal grouping of objects using dynamic programming #272
base: main
Are you sure you want to change the base?
Generic implementation of optimal grouping of objects using dynamic programming #272
Conversation
The algorithm will work on an array of objects if I am not wrong. So, the code should be shifted to |
I have moved the file to linear_data_structures. |
Hi, |
Hi, |
Add some tests for your patch. See |
|
||
|
||
|
||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too many empty lines.
""" | ||
gets a value | ||
""" | ||
return matrix[lookup_index[0]][lookup_index[1]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One liner functions aren't recommended. They just slow down the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my suggestions at https://github.com/codezonediitj/pydatastructs/pull/272/files#r426157438
You have to do something similar with every one liner function.
""" | ||
|
||
# gets the present value at the present index | ||
present_value = _get_value_opt_group(cost_storage, lookup_index) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
present_value = _get_value_opt_group(cost_storage, lookup_index) | |
present_value = cost_storage[lookup_index[0]][lookup_index[1]] |
def _test_lookup_function(lookup_index: List[int], input_index: List[int]): | ||
if lookup_index is None: | ||
raise TypeError( | ||
'Check lookup_function: returning wrong type should return an array of start and end index') | ||
|
||
if lookup_index.__len__() < 2: | ||
raise ValueError( | ||
'Check lookup_function:lookup index should at least have 2 integer items, first specifying the start and second specifying the last indices') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if input_index == lookup_index: | ||
raise RuntimeError( | ||
'Check lookup_function:verify get_lookup_fn giving same output as input which will lead to infinite loop') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if input_index == lookup_index: | |
raise RuntimeError( | |
'Check lookup_function:verify get_lookup_fn giving same output as input which will lead to infinite loop') | |
assert input_index == lookup_index |
if get_lookup_fn is None or type(get_lookup_fn) is not FunctionType: | ||
raise TypeError( | ||
'get_lookup_fn cannot be none and should be a function with 3 arguments') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not worry about type checking. If user will give garbage, they will get garbage.
@@ -3,13 +3,16 @@ | |||
from pydatastructs.utils.misc_util import _check_type, _comp | |||
from concurrent.futures import ThreadPoolExecutor | |||
from math import log, floor | |||
from types import * |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not do *
, import only those things which you want to use. Avoid namespace pollution.
Codecov Report
@@ Coverage Diff @@
## master #272 +/- ##
=============================================
- Coverage 98.842% 98.662% -0.181%
=============================================
Files 23 23
Lines 2420 2467 +47
=============================================
+ Hits 2392 2434 +42
- Misses 28 33 +5
|
Hi, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
@pravalikavis Please resolve merge conflicts. |
My only concern with dynamic programming problems is that they aren't that abstract and generalized. Moreover, they are pseudo-polynomial solutions to optimisation problems (mostly discrete). So possibly, we can add a separate optimisation module and put the dynamic programming solutions there. In fact they can also contain parametrised approximation algorithm. Anyways, we should first check if there is already a python package for solving these kind of formulations. |
Agreed on this |
References to other Issues or PRs or Relevant literature
fixes #230
Brief description of what is fixed or changed
Other comments