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

and/or between filters #1733

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

Prahitha
Copy link
Contributor

@Prahitha Prahitha commented Sep 1, 2021

The functionality is kind of limited and still needs some work. For example, it assumes that the user will not enter: coverage > 30 AND/OR coverage > 40 AND/OR coverage > 50

I would also like to know if the data structure used here is okay or if I should implement an array with priority like earlier. Any feedback is appreciated. Thank you!

Signed-off-by: Prahitha Movva <[email protected]>
Signed-off-by: Prahitha Movva <[email protected]>
@mglb
Copy link
Contributor

mglb commented Sep 1, 2021

This doesn't seem to work in basic cases:

  • (empty/default value) tool is icarus -> full table is visible;
    removing the empty value and leaving either OR or AND as the default should remove this problem.
  • (AND) tool is icarus -> OK
  • (OR) tool is icarus -> all tools visible with "no matching records"

"Remove all filters" doesn't remove filter from the table. Only filter fields are removed.

Why did you remove support for relation operators in entries (rows)? It has been useful to e.g. search "coverage <= 0 OR >= 100" AND "tool is X".

Code structure seems to be OK at first glance - lets make it work well first and optionally do some small optimization/refactoring later.

@Prahitha
Copy link
Contributor Author

Prahitha commented Sep 1, 2021

Thanks for the feedback, I'll work on those issues. Also, remove all filters works fine for me. It takes a awhile to redraw the table though.

Why did you remove support for relation operators in entries (rows)? It has been useful to e.g. search "coverage <= 0 OR >= 100" AND "tool is X".

@tgorochowik suggested that we have only one filter per row. So we agreed to first implement AND/OR between rows as a global relation and then extend it to AND/OR between every row.

Signed-off-by: Prahitha Movva <[email protected]>
@mglb
Copy link
Contributor

mglb commented Sep 2, 2021

Also, remove all filters works fine for me. It takes a awhile to redraw the table though.

Case that don't work in Chrome and Firefox:

  • Select OR
  • Add
  • Select: tool is icarus
  • Apply → "No matching records"
  • Remove all filters → still "No matching records"

I did wait a few minutes for results.

@Prahitha
Copy link
Contributor Author

Prahitha commented Sep 2, 2021

Ah yes, it doesn't seem to work when there are no matching records.

Signed-off-by: Prahitha Movva <[email protected]>
Comment on lines 128 to 131
function filter_cell(coverage, tool, types) {
var result = eval("(coverage, tool, types) => (" + eval(filter_expression) + ")");
return result;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm missing something here. When I print result in the console it shows (coverage, tool, types) => (true/false). I believe this is supposed to only return a boolean though. So, I was wondering what I was doing wrong and wanted some help. Thank you!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's break it:

var filter_result = eval(filter_expression) // <- boolean value
var function_code = "(coverage, tool, types) => (" + filter_result + ")";
var result = eval(function_code);

the last line is effectively the same as:

var result = (coverage, tool, types) => (filter_result); // either `... => (true)` or `... => (false)`

So this returns a function and the console prints its code.

(Spoiler alert: hints/solutions for further steps ahead ;) )

As a side note:
I guess filter_expression is a string with javascript expression code that uses coverage, tool, types variables. If so, you actually only need this inner eval (filter_result) to get result of the expression.
However, the whole function code looks like you want to actually generate a filter_cell function - in this case:

  • You only need the outer eval.
  • Instead of filter_result value, you should use the expression string (filter_expression) itself in the string concatenation.
  • The function filter_result doesn't need any of its arguments (or, for readability purposes, could take filter_expression string as an argument). More suitable name for the function would be something like compile_filter_cell_function. It should be used this way: var filter_cell = compile_filter_cell_function(filter_expression).

The difference between two solutions (using the function that does eval of the expression vs compiling this function) is performance - filter_cell will be called on a few hundred (thousand?) cells in a loop, so it's better to run eval once instead of running it for every cell.

Signed-off-by: Prahitha Movva <[email protected]>
Signed-off-by: Prahitha Movva <[email protected]>
Signed-off-by: Prahitha Movva <[email protected]>
@github-actions
Copy link
Contributor

github-actions bot commented Nov 9, 2021

Compared test results

tool new_failures new_passes added removed not_affected
Slang 0 0 0 0 4974
Verilator 2 1 0 0 4986
Yosys 0 1 0 0 4973
moore 0 0 0 0 4746
Icarus 0 0 0 0 4655
UhdmYosys 1 1 0 0 4972
Surelog 2 0 0 0 4972
Verible 0 0 0 0 4655
VeribleExtractor 0 0 0 0 4655
sv_parser 0 0 0 0 4746
Sv2v_zachjs 2 0 0 0 4972
tree_sitter_verilog 0 0 0 0 4655
Odin 0 0 0 0 4746
UhdmVerilator 0 0 0 0 4989

Download an archive containing all the details

@Prahitha Prahitha marked this pull request as ready for review November 16, 2021 04:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants