Fix suggestion, needs discussion #415
Conversation
eddiebergman
left a comment
eddiebergman
left a comment
There was a problem hiding this comment.
I don't have enough context remembered to really comment on this properly unfortunately.
I guess my only feedback is that if you're removing this logic, and this logic had a reason to exist, then document what bug/issue can come up as a result of taking out this logic.
If there is no such example, then this was dead code and good riddance to it :)
| parent_vector_idx: np.intp | Array[np.intp] | ||
| if isinstance(condition, Conjunction): | ||
| assert condition.parent_vector_ids is not None | ||
| parent_vector_idx = condition.parent_vector_ids | ||
| else: | ||
| parent_vector_idx = np.asarray(condition.parent_vector_id) | ||
|
|
||
| if np.isnan(vector[parent_vector_idx]).any(): | ||
| active = False | ||
| break | ||
|
|
There was a problem hiding this comment.
You could be less aggressive with the deletion and keep the else: branch, i.e. when it's not a Conjunction. Otherwise, I guess the fix is to see what kind of conjunction it is to know whether to use .any() [AND] or .all() [OR].
That being said, I don't know if this code is needed here in the first place.
There was a problem hiding this comment.
But the else branch no longer serves a purpose due to the if statement of 522 disappearing (condition.statisfied_by_vector is not using it)
There was a problem hiding this comment.
Turned it into a one liner for readability
For #194
This issue seems to be that or conditions are not verified correctly. I have turned the concise code summary of Eddie into a PyTest, and solved it afterwards.
The issue seems to be that conditions are first check if any of their parent values are nan; this makes sense for And Conjunctions (if nan the conjunction never evaluates to true; hence any and conjunction cannot be true) but nor for Or. For now I have removed it entirely. Alternatively, it can be re-added, under an if statement that first checks if we are verifying an AndConjunction.