&Nidhicodes [ENH] Add support for nn losses to ptf-v2 - #2331
Conversation
…ytorch-forecasting into nidhi_nn_losses
Signed-off-by: Faakhir30 <zahidfaakhir@gmail.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2331 +/- ##
=======================================
Coverage ? 87.71%
=======================================
Files ? 177
Lines ? 10450
Branches ? 0
=======================================
Hits ? 9166
Misses ? 1284
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
phoeenniixx
left a comment
There was a problem hiding this comment.
Can we try this adpater with some already implemented methods (optimally all of them, but that is hard, ig we need to update our test framework so that it loops over the losses like we do for v1)
I think you are referring to
v1 has Even if I add that tag, are you suggesting to maintain all set of nn.losses like we had sets of metrics at I think it'll be better to just add a few nn losses to each |
Signed-off-by: Faakhir30 <zahidfaakhir@gmail.com>
|
Sorry I meant models, not methods |
Signed-off-by: Faakhir30 <zahidfaakhir@gmail.com>
Signed-off-by: Faakhir30 <zahidfaakhir@gmail.com>
248ccf6 to
de4912c
Compare
phoeenniixx
left a comment
There was a problem hiding this comment.
Can you please add docstrings in the adapter - without it, it is hard to review!
Also, what if a user uses this: loss = Multiloss([MAPE,nn.MSELoss])?
Should we add some way so that if we are even using a ptf loss with nn loss, this should work
Signed-off-by: Faakhir30 <zahidfaakhir@gmail.com>
Signed-off-by: Faakhir30 <zahidfaakhir@gmail.com>
…h-forecasting into nidhi_nn_losses
Ahh, I did think about this at start, I was thinking if instead of taking this adapter's base from But that design automatically makes a nn loss interchangeable with any Metric losses without much friction. I should have shared this intuition in PR description as alternative before ;) I did not point out initially, because on @Nidhicodes PR, I dont see any maintainers comment pointing above problem. Is there something I'm missing? is there some major issue with |
|
sorry, I dont exactly understand what your question is here, can you please rephrase it? |
Signed-off-by: Faakhir30 <zahidfaakhir@gmail.com>
I was suggesting an alternative design. Design alternative I'm inclined towards: instead of Question: I have pushed this newer design in a single commit for review, we can revert back to keeping adapter a |
phoeenniixx
left a comment
There was a problem hiding this comment.
Added some comments
- Please use
if-elserather than multipleifstatements if possible. This is not only useful to prevent looking at unnecessaryifstatements but also makes it clear for the reader that the statement below is related to the aboveifand not a totally different condition. In short, makes it easier to understand code
Signed-off-by: Faakhir30 <zahidfaakhir@gmail.com>
6443b3e to
9e6ff22
Compare
Reference Issues/PRs
Fixes #1970
What does this implement/fix? Explain your changes.
Followups and stacks on existing PR #2073 by @Nidhicodes
By @Nidhicodes:
The adapter:
Further updates:
self._lossinstead ofself.losstideand other models where neededWhat should a reviewer concentrate their feedback on?
Did you add any tests for the change?
Any other comments?
PR checklist
pre-commit install.To run hooks independent of commit, execute
pre-commit run --all-files