- 
                Notifications
    
You must be signed in to change notification settings  - Fork 1.3k
 
exp show: Add --drop and --keep args #7141
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
Conversation
02c2675    to
    94a8c83      
    Compare
  
    | 
           How does filtering with the new options work if there is a metric and parameter with the same name?  | 
    
| 
           Cool, I don't like the old   | 
    
          
 User need to indicate the pattern matching the final column. So if there is a shared name, user would need to   | 
    
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.
Looks great! Nice simplification of the UI.
The big question is whether removing --include flags will make it too hard to filter the table. Now that we have --only-changed, the hope is that this will cover a lot that filtering automatically (along with --drop).
We haven't had much feedback recently on the table filtering, so let's try it and see what people think.
          
 I was thinking that if we support  I'm Ok with adding regex and just examples in docs. @dberenbaum WDYT?  | 
    
          
 I knew we had this discussion before π : #5720 (comment). No strong preference from me. Not sure if regex is less consistent with the rest of the dvc cli, but obviously it's more powerful, so π€· .  | 
    
2954e0f    to
    d6da3ef      
    Compare
  
    | 
           @daavoo this needs a rebase, exp commands have been split into their own files (so   | 
    
d6da3ef    to
    31c80dd      
    Compare
  
    
          
 Done. I had already done that one but this required a new one because of the parallel coordinates merge  | 
    
31c80dd    to
    e65a863      
    Compare
  
    | 
           Nice. On first sight it's not clear to me what the difference between include and keep are but we'll worry about that in the docs PR. Just one Q: Does the order in which filters are given matter? I.e. if I keep something and then drop the same something what happens? I'd even try  UPDATE: I guess it's already explained in iterative/dvc.org#3112 so nvmd.  | 
    
d3837ab    to
    0b00d17      
    Compare
  
    802dfd1    to
    c8582f9      
    Compare
  
    c8582f9    to
    556a6db      
    Compare
  
    | 
           This pull request introduces 1 alert when merging 556a6db97d321d86cf56bfe8df8899e199aeb2f4 into 0a4958b - view on LGTM.com new alerts: 
  | 
    
556a6db    to
    22829cd      
    Compare
  
    22829cd    to
    2035b90      
    Compare
  
    TabularData: Add subset arg to dropna and drop_duplicates. Closes #7083
Removed `--include-metrics` / `--include-params` and `--exclude-metrics` / `--exclude-params`. Removed `--no-timestamp` . Can be done by `--drop Created`. `--drop` and `--keep` operate directly on the table columns. `--keep` does not perform any filtering. It's only used to specify columns to keep despite the other filtering. For example `--only-changed --keep foo` will prevent `foo` from being removed by `--only-changed`. Another example, `--drop train.* --keep train.dropout` will remove all columns matching `train.*` except for `train.dropout`. Closes #7079 Closes #7080 Pre-requisite for #6434
2035b90    to
    11c687a      
    Compare
  
    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, in the most parts.
| to_remove = set() | ||
| for col in col_names: | ||
| if not self.is_protected(col): | ||
| to_remove.add(col) | 
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.
How about remove to_remove and directly:
        for col in col_names:
            if not self.is_protected(col):
                self._keys.remove(col)
                self._columns.pop(col)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.
It merged automatically.
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.
β I have followed the Contributing to DVC checklist.
π If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.
Removed
--include-metrics/--include-paramsand--exclude-metrics/--exclude-params.Removed
--no-timestamp(->--drop Created)--dropand--keepoperate directly on the table columns.Support regex patterns.
--keepdoes not perform any filtering.It's only used to specify columns to keep despite the other filtering.
Previous
--includebehavior can be replicated with negative lookahead regex.Closes #7079
Closes #7080
Closes #7083
Pre-requisite for #6434
Example:
`dvc exp show`
`dvc exp show --only-changed`
`dvc exp show --only-changed --keep train.batch_size`
`dvc exp show --drop train.*`
`dvc exp show --drop train.* --keep train.dropout`
`dvc exp show --only-changed --drop Created`
For #7080
`dvc exp show --only-changed --drop Experiment --drop Created`