Skip to content
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

Score fixed #485

Open
wants to merge 4 commits into
base: development
Choose a base branch
from
Open

Score fixed #485

wants to merge 4 commits into from

Conversation

dengdifan
Copy link
Contributor

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Note that a Pull Request should only contain one of refactoring, new features or documentation changes.
Please separate these changes and send us individual PRs for each.
For more information on how to create a good pull request, please refer to The anatomy of a perfect pull request.

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally?

Description

Fix #484 to allow the forecasting-related arguments to be passed to compute task.score

Motivation and Context

How has this been tested?

@codecov
Copy link

codecov bot commented Dec 8, 2022

Codecov Report

Base: 85.49% // Head: 84.50% // Decreases project coverage by -0.98% ⚠️

Coverage data is based on head (1e2180b) compared to base (d160903).
Patch coverage: 80.00% of modified lines in pull request are covered.

Additional details and impacted files
@@               Coverage Diff               @@
##           development     #485      +/-   ##
===============================================
- Coverage        85.49%   84.50%   -0.99%     
===============================================
  Files              231      232       +1     
  Lines            16351    16475     +124     
  Branches          3028     2734     -294     
===============================================
- Hits             13979    13923      -56     
- Misses            1533     1675     +142     
- Partials           839      877      +38     
Impacted Files Coverage Δ
autoPyTorch/datasets/time_series_dataset.py 90.63% <ø> (ø)
setup.py 0.00% <ø> (ø)
autoPyTorch/api/time_series_forecasting.py 66.29% <77.27%> (+3.60%) ⬆️
autoPyTorch/api/base_task.py 82.60% <100.00%> (-1.22%) ⬇️
...luation/time_series_forecasting_train_evaluator.py 90.62% <100.00%> (-0.18%) ⬇️
autoPyTorch/pipeline/tabular_regression.py 69.30% <0.00%> (-13.87%) ⬇️
autoPyTorch/ensemble/ensemble_selection.py 85.41% <0.00%> (-11.46%) ⬇️
autoPyTorch/evaluation/abstract_evaluator.py 66.40% <0.00%> (-11.00%) ⬇️
autoPyTorch/evaluation/utils_extra.py 70.58% <0.00%> (-8.83%) ⬇️
autoPyTorch/pipeline/tabular_classification.py 75.20% <0.00%> (-6.62%) ⬇️
... and 40 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@@ -1791,6 +1792,8 @@ def score(
The test predictions
y_test (np.ndarray):
The test ground truth labels.
score_kwargs: Any
additional arguments for computing the scores. Some metrics might require special arguments
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you add a note saying something like: Currently, needed for time series forecasting tasks. See autoPyTorch/api/time_series_forecasting.py

if isinstance(seasonality, list):
seasonality = min(seasonality) # Use to calculate MASE
self.seasonality = int(seasonality) # type: ignore[call-overload]
self.seasonality = self.datamanager.seasonality
Copy link
Contributor

@ravinkohli ravinkohli Dec 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the AbstractEvaluator class, we use a _init_datamanager_info (see here). At the end of this method we delete the datamaneger object to reduce the memory usage. Could you also update the time series forecasting evaluator, maybe overriding the parent class function?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can also do it as a separate PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

data manager is required somewhere else by the time series evaluator because I defined time series dataset's getitem differently. Maybe in the future, we could fix that. But currently, I would prefer to keep self.datamanager in the evaluator

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay sure.

Copy link
Contributor

@ravinkohli ravinkohli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your changes. I think the tests are failing because we need to update the requirements. Particularly, gluonts version 0.11.4 does not have 'DayOfMonth' from 'gluonts.time_feature'. Could you take a look?

@dengdifan
Copy link
Contributor Author

dengdifan commented Dec 19, 2022

Thanks for your changes. I think the tests are failing because we need to update the requirements. Particularly, gluonts version 0.11.4 does not have 'DayOfMonth' from 'gluonts.time_feature'. Could you take a look?

This is fixed. However, I am considering removing all these dependencies (gluonts, pytorch-forecasting) in the future (which might take some time, though).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants