Feature/rule optimiser #6
Conversation
… not( and( 1, 2, 3, ) ). Coming soon
…ry in advance for anyone reading this. It works tho on most edge cases.
…be run in two lines.
…of the RuleOptimiser
…ersonnames, addresses, etc ). These rules get converted to numbers and then back to rules when the script had done its magic
src/os2datascanner/projects/admin/adminapp/templates/miniscan.html
Outdated
Show resolved
Hide resolved
| def list_equality_no_order(arr1, arr2): | ||
| """In python [1, 2, 3] == [3, 2, 1] equals False. | ||
| In our case, we just want to check if two containers | ||
| have the same components. This function detects any | ||
| elements that do not occur in both. If none are found, | ||
| they are identical.""" | ||
| base = arr1 | ||
| for k in arr2: | ||
| try: | ||
| base.remove(k) | ||
| except ValueError: | ||
| # Can't remove because it isn't there | ||
| return False | ||
| # Return not (is there anything left in base). True if empty else False | ||
| return not base |
There was a problem hiding this comment.
You work around this by doing .copy() on the input argument in the one place you call this function, but an equality comparison that destroys one of its operands is a Bad Surprise.
(The logic here is also unnecessarily complicated -- you could just do sorted(arr1) == sorted(arr2).)
There was a problem hiding this comment.
Unfortunately, sorting the list doesn't work, since it contains (at least can contain) CustomContainer classes, that can't be sorted. Hence my code. .copy() works though.
| self.reformat_rule(OUT_PATH) | ||
|
|
||
|
|
||
| class CustomContainer(): |
There was a problem hiding this comment.
Why are you doing your own JSON parsing when the os2datascanner.engine2.rules package exists? 🤔
There was a problem hiding this comment.
I didnt know about the rules package when doing this. Also, did you mention the wrong piece of code or am i missing something in the 4 quoted lines ?
There was a problem hiding this comment.
This file is an output file from your tool and doesn't belong in the source hierarchy. (This is also an example of the risks of having a hard-coded output path in the source hierarchy!)
There was a problem hiding this comment.
Yes, i wrote a comment about having this in a tmp directory and removing it once done. Is there a place for temporaryu resources, or do i just go to the resources folder and remove it after ?
There was a problem hiding this comment.
Another file that shouldn't be here.
There was a problem hiding this comment.
Same question as prior
| return True | ||
| return False | ||
|
|
||
| def optimisation_cycle(self): |
There was a problem hiding this comment.
This optimisation pass looks vaguely plausible, but you haven't implemented any unit tests for it, so it's hard to see what it actually does...
There was a problem hiding this comment.
Maybe i don't see the same quoted code as you do or you made a mistake, or do you mean in general ?
| def check_useless(self, container:'CustomContainer'): | ||
| # Useless i.e. AND(1), OR(1), etc ... | ||
| if len(container.components) == 1: | ||
| try: | ||
| container.parent.components = arr_switch(container.parent.components, container, container.components[0]) | ||
| self.containers.remove(container) | ||
| self.found_redundancy = True | ||
| except ValueError: | ||
| pass | ||
| return True | ||
| return False |
There was a problem hiding this comment.
The os2datascanner.engine2.rules package already handles this case. If you try to make a CompoundRule with a single component, it automatically decays to that component:
@classmethod
@abstractmethod
def make(cls, *components):
"""Creates a new Rule that represents the combination of all of the
given components. The result does not need to be an instance of @cls:
it could be a completely different kind of Rule, or a simple boolean
value, if this rule can already be reduced to one.
Subclasses must override this method, but should call up to this
implementation."""
if len(components) == 0:
raise ValueError("CompoundRule with zero components")
elif len(components) == 1:
return components[0]
else:
return cls(*components)That is, we already implicitly do this optimisation when we execute the rule.
(That's not to say necessarily that you shouldn't do it elsewhere too, but what if this was deliberate? What if you define a rule with an AndRule to leave yourself a hook to add other things later?)
There was a problem hiding this comment.
Your point here is that this test is useless, because the rule_builder does it before compiling it and sending to be checked at the scanner ? If so i can just delete my check ?
|
It seems that the optimiser only works with JSON one-liners ... |
Optimises rules for miniscanner / datascanner entered by the user. Removes redundancies such and cpr AND cpr.