-
Notifications
You must be signed in to change notification settings - Fork 691
[ENH] Improve test framework for v1 models #1908
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?
Conversation
For now only |
|
||
all_train_kwargs.append(final_params) | ||
train_kwargs_names.append(f"base_params_{i}_{loss_name}") | ||
else: |
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 if-else
block is just added so that the model_pkg
classes that still not have the tag info:compatible_loss
can also pass the tests, I will remove it once all the model_pkg
classes have this tag
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1908 +/- ##
=======================================
Coverage ? 86.61%
=======================================
Files ? 97
Lines ? 7865
Branches ? 0
=======================================
Hits ? 6812
Misses ? 1053
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
final_params = deepcopy(base_params) | ||
for key, value in loss_params.items(): | ||
if ( | ||
isinstance(value, dict) |
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.
5 or more tabs is a code smell - can we avoid this?
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.
sure, I moved the code to different functions
This is strange, the tests are passing for macOS and ubuntu and failing for windows?
When I run locally this doesnot happen. Although I am on ubuntu so I cannot say much, maybe someone with windows test what the issue is? |
Is it always the same test, I am suspecting there is something with numerical accuracy happening, e.g., values that are close to zero but not quite zero. |
Yes
I'll then shift to windows and will see locally what exactly is happening |
one trick is also to introduce prints or artificial raises in the code that trigger only when you expect the error - although in this specific case I fear that is not going to be easy, so trying to reproduce locally might be your best bet. |
Strange.
|
Hi, so Then maybe the issue is different float types as you said on discord, but as we know ( and I checked) Then I found that in def map_x_to_distribution(self, x: torch.Tensor) -> distributions.LogNormal:
scale=F.softplus(x[..., 1])
return self.distribution_class(loc=x[..., 0], scale=scale) And obviously this also didn't work. After sometime, I found that somehow From this exprience I learned one thing atleast, that debugging should be "targetted" and not just random guesses, I think my random gusses also have made this process so complex. Edit: just to make it clear and add more context how the y = (tensor([[1., 1., 1.],
[1., 0., 0.]]), None) and in some cases (when I would rerun the debugger), the value is something like: y = (tensor([[1., 1., 1.],
[1., 1., 0.]]), None) or y = (tensor([[1., 1., 1.],
[1., 1., 1.]]), None) In the second case, (when there is no 0), in some other |
To see if the float types Changes after transformation to tensors, I first need to find the place where this transformation is being done and there I can add |
to quickly summarize our discussion in the tech meet today: the It is also proving that the test framework works well! Since it seems to be a genuine issue, rather than something to do with the framework - and the framework is intended to increase coverage so these combination cases get picked up. The approach I would recommend is to park detected failures that are not immediately clear how to fix, and move them to issues - after all, the failures were there before, just not detected. Add some skipping mechanism to skip precisely the failure, open an issue with a description, and then we can deal with it later - and turn the test back on in the same PR as the fix (this pointer should be there in the issue) This approach is a corollary of "test driven development" where you start with tests that fail, and then gradually change your code to make the tests pass - in separate PR and step-wise. If you write new code, then all tests fail initially; but we are improving coverage and consistency of an existing code base, so only some tests will fail in this case. |
Reference Issues/PRs
Fixes #1906
What does this implement/fix? Explain your changes.
The PR makes the changes as suggested in #1906:
"info:compatible_loss"
tomodel_pkg
classes to keep a track of losses a model is compatible with."distr"
- Distribution Losses like NormalDistributionLoss"quantile"
- QuantileLoss"point"
- Point Prediction Losses like MAEMultiLoss
containing only losses compatible with a given model.