-
Notifications
You must be signed in to change notification settings - Fork 5
WPL #37
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
base: main
Are you sure you want to change the base?
WPL #37
Conversation
|
| WPL_params.add_argument( | ||
| "--default_scale", | ||
| nargs="+", | ||
| type=float, | ||
| default=[1.0], | ||
| help="Weibull scale to use when the number of uniqe element is less than 5 default: %(default)s", | ||
| ) | ||
| WPL_params.add_argument( | ||
| "--default_shape", | ||
| nargs="+", | ||
| type=float, | ||
| default=[0.1], | ||
| help="Weibull shape to use when the number of uniqe element is less than 5 default: %(default)s", | ||
| ) |
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.
If these parameters are not supposed to be grid searched, they should not be a list rather floats. And if you are planning to grid search them, it should be noted that these do not impact the training in any way, since these are just the default values that need to be returned in a corner case. Possibly, it would be better if this was handled by whatever process is calling the inference function.
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.
default_scale and default_shape are similar to tail_size, cover_threshold, and distance_multiplier of EVM. Please check the EVM.py in the main repo.
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.
Algo Breakdown: Please try to simplify or break down your algorithm. Your WPL algo has two parts one is per dimension weibull and the other is how you address the case where weibull is needed for less than 5 samples/values. I am asking to separate this part from your core algorithm.
If tomorrow someone comes up with a new way to estimate default_scale and default_shape rather than fixing default values (let's say a mean from other class values, which can't be found until the algorithm has been run on the entire dataset) would you create a new WPL.py and call it a new algorithm even though the per dimension weibull idea is the same?
Reducing Computation: default_scale and default_shape are not similar to tail_size, cover_threshold, and distance_multiplier of EVM. These EVM parameters were being used as a list to reduce redundant computation. In your case, you will be recalling the mr.FitHigh function using the same parameters when your default_scale and default_shape change, even though they do not impact the result in this case. That is you are adding computational overhead rather than reducing it.
default_scale and default_shape are simply being used to replace scale and shape values where none could be found, not reduce computation.
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.
Both of the comments Algo Breakdown and Reducing Computation are correct. @scruz13 please make required changes.
|
|
||
|
|
||
| def fit_high(distances, distance_multiplier, tailsize, default_shape, default_scale): | ||
| distances = torch.unique(distances) |
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.
distances is always supposed to be a 2D tensor i.e.
no_of_samples x no_of_samples_to_which_distance_was_found
so the dim should be used when using unique
Also unique by default returns the sorted tensor, which might be redundant work taken care of in weibull.py
This seems to be trying to address a bigger problem, should it not be addressed in weibull.py?
@tboult: If we remove multiple occurrences of the same values, won't it result in different weibull models.
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.
In the WPL, we have a prototype. i.e., no_of_samples_to_which_distance_was_found == 1 .
It is better to do not change weibull.py because it affects EVM and our papers
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.
Even in openmax no_of_samples_to_which_distance_was_found=1 but still its fit_high function supports working on a 2D tensor absence of dim in your code above removes that support.
I will let you and @tboult decide if you want to incorporate this in weibull.py it is very much possible to incorporate this there without changing the default behaviour for any of the other algorithms. At least changing it there helps to quickly test the same idea for other algorithms as well.
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.
Correction: I made an incorrect suggestion above, dim with unique doesn't do what I expected it to. So using unique in fit_high will by default remove the support for computation on a 2D tensor.
| tailsize = int(min(tailsize, distances.shape[1])) | ||
| mr = weibull.weibull() | ||
| if distances.shape[1] < 5: | ||
| pass |
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 might be better to simply create a weibull object where scale and shape tensors are nan and return it. The end-user can simply replace the nan values with the defaults you have in args.
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.
args contains scale and shape of Weibull. So, why do not using it here? Why do you want to end-user replace it?
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.
Please check my comment above #37 (comment)
vast/opensetAlgos/WPL.py
Outdated
| with torch.no_grad(): | ||
| for pos_cls_name in pos_classes_to_process: | ||
| features = features_all_classes[pos_cls_name].clone().to(f"cuda:{gpu}") | ||
| assert args.dimension == features.shape[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.
This is the first mention of dimension and it is not in your WPL_params function.
Also, why not just initialize it here and run for all dimensions by default, rather than leaving it as an argument?
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.
changed.
vast/opensetAlgos/WPL.py
Outdated
| assert args.dimension == features.shape[1] | ||
|
|
||
| center = torch.mean(features, dim=0).to(f"cuda:{gpu}") | ||
| distances = torch.abs(features - center.view(1,args.dimension).repeat(features.shape[0], 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.
Looks like you are taking the L1 distance, please consider putting it in pairwisedistances.py and using it as was done in openmax.py, this ensures the same code being run in training and inference and also simplifies changing the distance computation by simply changing the argparse variable.
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 is absolute value of feature from center (prototype). It cannot replace by other metric. For example, cosine distance of two scalar is not defined.
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, you are right that this might not be replaceable by other metrics and neither would be a useful metric for other algorithms, which is what I was thinking.
BTW, you do not need .repeat above, even if needed it should be replaced with expand as it reduces memory consumption.
vast/opensetAlgos/WPL.py
Outdated
| args.tailsize, args.distance_multiplier, args.default_shape, args.default_scale | ||
| ): | ||
| weibull_list = list() | ||
| for k in args.dimension: |
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 for loop is not needed, you can just send the two-dimensional distance tensor into fit_high. This can reduce the compute time both during training and inference. This also means you will not need the weibull_list variable anymore.
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 confused! Does fit_high return a Weibull or list/tuple of many Weibulls?
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.
You should also be able to use it to return multiple weibulls at once. If distances is a nxk tensor, where in your case n is the number of dimensions, then it can straightaway provide you n weibulls in a single mr object which will reduce both training and inference time.
vast/opensetAlgos/WPL.py
Outdated
| distances = torch.abs(test_cls_feature - center.view(1,args.dimension).repeat(test_cls_feature.shape[0], 1)) | ||
| weibull_list = models[class_name]["weibull_list"] | ||
| p = torch.empty(args.dimension) | ||
| for k in args.dimension: |
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.
As mentioned in the training function this for loop is avoidable.
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.
Check comment above. #37 (comment)
| mr.sign = 1 | ||
| mr.wbFits = torch.zeros(1, 2) | ||
| mr.wbFits[0, 1] = default_scale | ||
| mr.wbFits[0, 0] = default_shape | ||
| mr._ = torch.Tensor(0.0) # translate Amount | ||
| mr.smallScoreTensor = torch.Tensor(0.0)# small Score |
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.
You could just create this as done at
vast/vast/opensetAlgos/multimodal_openmax.py
Line 119 in 460480f
| mr = weibull.weibull( |
It might make it more readable
| dimension = None | ||
| for pos_cls_name in pos_classes_to_process: | ||
| features = features_all_classes[pos_cls_name].clone().to(f"cuda:{gpu}") | ||
| if dimension == None: | ||
| dimension = features.shape[1] | ||
| else: | ||
| assert dimension == features.shape[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.
dimension will always be None so will never reach the else statement.
| test_cls_feature = features_all_classes[batch_to_process].to(f"cuda:{gpu}") | ||
| assert test_cls_feature.shape[0] != 0 | ||
| probs = [] | ||
| for cls_no, cls_name in enumerate(models.keys()) |
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.
While you expect the models variable to be an ordered dict if a user instead passes a dict rather than letting the user know this will simply provide them with incorrect results. This can be avoided by simply using enumerate(sorted(models.keys())) instead of enumerate(models.keys())
Weibull Prototype Learning (WPL)