-
Notifications
You must be signed in to change notification settings - Fork 6
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
feat: search results - evaluation and results processing #249
base: main
Are you sure you want to change the base?
Conversation
…nable logging with objective names.
pyproject.toml
Outdated
@@ -21,7 +21,8 @@ dependencies = [ | |||
"torchvision>=0.18", | |||
"boto3==1.34.147", | |||
"botocore==1.34.147", | |||
"deepspeed" |
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.
maybe let's revert this here and rather merge the main branch once #248 is in
whittle/search/search.py
Outdated
"runtime": runtime[-1], | ||
} | ||
|
||
print(observation) |
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.
remove print
whittle/results/search/plot.py
Outdated
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.
This file seems somewhat specific to our paper. Should we have that in a separate repo?
whittle/results/__init__.py
Outdated
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.
This seems also specific to our experiments. Maybe move that in a separate repo?
|
||
|
||
@dataclass | ||
class ParamBinArgs: |
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.
Shouldn't that be part of SearchArgs?
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 can imagine that you'll use binning outside of search (e.g. for pre-selection of networks to evaluate). But I can add it to SearchArgs
whittle/search/search.py
Outdated
@@ -19,6 +20,9 @@ def multi_objective_search( | |||
objective_kwargs: Optional[dict[str, Any]] = None, | |||
logger: Optional[Logger] = None, | |||
seed: Optional[int] = None, | |||
param_bins: Optional[ParamBins] = None, |
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 am wondering if it wouldn't be cleaner to have this in the search method instead of the objective? We could have a something like stratified random search that samples sub-networks uniformly across parameter count instead of uniformly from the search space. Wdyt?
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.
Yes, great idea
@@ -15,6 +15,7 @@ def compute_flops( | |||
batch_size: int = 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.
same nitpick as above
whittle/metrics/latency.py
Outdated
import torch | ||
import torch.profiler | ||
from torch.profiler import record_function | ||
from typing import Optional | ||
|
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.
import sorting
import json | ||
import pandas as pd | ||
from pathlib import Path | ||
from tqdm import tqdm | ||
from typing import Optional, Any |
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.
import sorting
def setup( | ||
results_dir: Path, | ||
output_path: Optional[Path] = None, | ||
pareto_path: Optional[Path] = None, | ||
) -> None: |
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.
same as above
whittle/results/search/plot.py
Outdated
import matplotlib.pyplot as plt | ||
import seaborn as sns | ||
import pandas as pd | ||
from pathlib import Path | ||
from typing import Optional |
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.
import sorting
…ction to a search strategy. Revert pyproject.toml
test/test_search.py
Outdated
bins, _ = ( | ||
param_bins(10, 2, 1) if search_strategy == "stratified_random_search" else None | ||
) | ||
|
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.
bins, _ = ( | |
param_bins(10, 2, 1) if search_strategy == "stratified_random_search" else None | |
) | |
bins, _ = ( | |
param_bins(10, 2, 1) | |
if search_strategy == "stratified_random_search" | |
else (None, None) | |
) |
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.
else this will throw an error
… both litgpt-like checkpoints and whittle subnets. Fix bins in test.
whittle/evaluate_network.py
Outdated
metrics["flops"] = compute_latency(model) | ||
if measure_latency: | ||
metrics["latency"] = compute_flops( | ||
model, batch_size=latency_batch_size, previous_device=device | ||
) |
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.
flops and latency functions are switched
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.
Nice catch!
Reference Issues/PRs
#230 + additional features in the search process
What does this implement/fix? Explain your changes.
objective_1
,objective_2
)By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.