[ENH] Move BaseFixtureGenerator to pytorch-forecasting/tests/_base
#1919
Conversation
PranavBhatP
changed the title
BaseFixtureGenerator to a pytorch-forecasting/tests/_baseBaseFixtureGenerator to pytorch-forecasting/tests/_base
|
FYI @phoeenniixx @fkiraly |
| # call _generate_estimator_class to get all the classes | ||
| all_model_pkgs, _ = self._generate_object_pkg(test_name=test_name) | ||
|
|
||
| all_cls = [est.get_model_cls() for est in all_model_pkgs] |
There was a problem hiding this comment.
I think here either we could change the name from get_model_cls to get_cls (this would work for both metrics and models) or
We could implement the _generate_obj_class, _generate_object_pkg and _generate_trainer_kwargs independently in respective child classes for metrics and models.
I am a little biased towards the second option as the logic to generate instances, params etc are little different for metrics and models, imo.
This is because in models, base-params are paired with different losses, and idts this is the case with metrics, they are quite independent from the models (@PranavBhatP please correct me if I am wrong!)
There was a problem hiding this comment.
Yes, ur reasoning makes sense, object_class and object_pkg are mostly having similar logic for both metrics and models but again, your changes might require having child class-specific implementations for the _generate_<fixture-name> methods.
There was a problem hiding this comment.
Yes, to generate base_params (from model_pkg) paired with each compatible loss and also to add test skips for specific fixture, fixture generation functions will have a little different logic as compared to metrics.
|
@phoeenniixx do I completely remove the I am okay with any workflow since |
Well, imo, the best way is to move |
|
I think the way you are doing right now (completely removing the Any change that should happen, imo, in the current state should be the removal of fixture generation functions and leaving them to the child fixture generators for implementation. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1919 +/- ##
=======================================
Coverage ? 86.44%
=======================================
Files ? 96
Lines ? 7828
Branches ? 0
=======================================
Hits ? 6767
Misses ? 1061
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:
|
the previous commit would have left to lots of merge conflicts with existing workflows and hence it is decided to only move the fixture generator as a true base class to _base
PranavBhatP
requested review from
benHeid,
fkiraly,
fnhirwa,
jdb78 and
yarnabrina
as code owners
|
@phoeenniixx @fkiraly I think this PR is ready for review and merge. To ensure minimal conflicts with the existing copy of |
PranavBhatP
moved this from PR in progress
to PR under review
in May - Sep 2025 mentee projects
What does this implement/fix? Explain your changes.
Currently we use the
BaseFixtureGeneratorfor generating fixtures for tests involving estimators and recently metrics (#1907). But the current import location is insidetest_all_estimators.py. This PR aims to make this class a true "base" class by moving it to a new location. This will be stacked upon by #1907 and #1908 to respectively implement child classes which inherit from this base class.Current state of PR does not implement a full base class, it is just a copy-paste for now, next steps to ensure that the base class can be extended as a fixture generator for both models and metrics.
PR checklist
pre-commit install.To run hooks independent of commit, execute
pre-commit run --all-files