-
Notifications
You must be signed in to change notification settings - Fork 24
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
🔨 Rework for Probabilistic Regression & Pixel Regression #125
Conversation
alafage
commented
Nov 28, 2024
•
edited
Loading
edited
- Rework Distribution Layers to be classifiers returning distribution parameters in a dictionary.
- Update RegressionRoutine
- Update PixelRegressionRoutine
- Update Tutorials
- Update API Reference
- Fix DistributionNLL (loss / metric)
- Update BTS baseline
- Add tutorial for simple probabilistic regression
- Update existing tests
- Write tests
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## dev #125 +/- ##
=======================================
Coverage 99.21% 99.22%
=======================================
Files 142 142
Lines 6792 6835 +43
Branches 849 857 +8
=======================================
+ Hits 6739 6782 +43
Misses 24 24
Partials 29 29
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
format and check using ruff
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.
Some comments on the documentation mainly!
Thanks for improving this part that wasn't that great (Sorry 🤐 )
) | ||
self.min_scale = min_scale | ||
|
||
def forward(self, x: Tensor) -> dict[str, Tensor]: |
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: | ||
x (Tensor): The input of the model. | ||
|
||
Returns: |
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.
Detail the return type?
loss (nn.Module): Loss function to optimize the :attr:`model`. | ||
dist_family (str, optional): The distribution family to use for | ||
probabilistic pixel regression. Defaults to ``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.
If None, pointwise regression
@@ -76,7 +79,9 @@ def __init__( | |||
) | |||
|
|||
self.model = model | |||
self.probabilistic = probabilistic | |||
self.dist_family = dist_family | |||
self.dist_estimate = dist_estimate |
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.
Why is self.dist_estimate
here and not in pixel-wise regression?
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.
That's a mistake, thanks!
Thank you for the great review! This last commit should address all your worries. Feel free to merge it into dev anytime! |
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.
Thanks for the changes! Just saw a small typo but we're good to go!
Depending of the :attr:`dist_layer` of the backbone, the output can | ||
be a distribution or a single tensor. | ||
Depending of the :attr:`dist_family` of the backbone, the output can | ||
be a dictionnary of distribution parameters or a single tensor. |
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.
dictionary