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

[ENH] Second test parameter set for Kalman Filter #6095

Merged
merged 5 commits into from
Mar 16, 2024

Conversation

XinyuWuu
Copy link
Member

@XinyuWuu XinyuWuu commented Mar 9, 2024

Toward #3429

Added the second test parameter set for KalmanFilterTransformerFP and KalmanFilterTransformerPK under sktime/transformations/series/kalman_filter.py

The physical meaning of the second test parameter set can be found in wikipedia example. Basicly, it is a one dimension position estimation with process noise and measurement noise. The state of the filter is [position, velocity] and $\Delta t=0.1$.

@XinyuWuu XinyuWuu changed the title Second test parameter set for Kalman Filter [ENH] Second test parameter set for Kalman Filter Mar 9, 2024
@fkiraly fkiraly added module:transformations transformations module: time series transformation, feature extraction, pre-/post-processing enhancement Adding new functionality labels Mar 9, 2024
@XinyuWuu
Copy link
Member Author

XinyuWuu commented Mar 10, 2024

Now it should pass the black formating check.

image

@XinyuWuu
Copy link
Member Author

I tried to fix the dimension problem. Now check_estimator(kf.KalmanFilterTransformerFP) will pass, but I can't get check_estimator(kf.KalmanFilterTransformerPK) pass.
The params I added is the same for KalmanFilterTransformerFP and KalmanFilterTransformerPK with state_dim = 2 and measurement_dim = 1.
As the document tells me, measurement_function = np.array([[1, 0]]) with shape (measurement_dim, state_dim) and measurement_noise = np.array([[0.1]]) with shape (measurement_dim, measurement_dim).
When check_estimator, X with shape (11,1) and (10,2) is passed to function _em in kalman_filter.py line 640 in two different tests. So the measurement_dim is X.shape[1] which is 1 and 2 in the two different tests. In my present test params, measurement_dim = 1 will not pass the second test. After I changed my test params to match measurement_dim = 2, it still will not pass the first test. How could I fix this? Is is a error with how we test the estimator?

@XinyuWuu
Copy link
Member Author

I have noticed your @fkiraly PR #6114 to revert switch from pykalman to pykalman-bardo, should we resolve #6114 first?

@fkiraly
Copy link
Collaborator

fkiraly commented Mar 14, 2024

@Xinyu-Wu-0000 - well spotted!

Originally, I was actually thinking, to make things convenient for you, we were going to merge this first, and then I'll work "around you", i.e., take care of all the unpleasant merge conflicts.

However, @mbalatsko did respond to the query, and said he was going to take care of the python 3.11/3.12 patch in pykalman, and that might take longer than the current release schedule, so I was thinking to delay the revert to 0.28.0 or 0.29.0, in order to not have a gap where pykalman based estimators have no 3.11/3.12 support.

@fkiraly
Copy link
Collaborator

fkiraly commented Mar 14, 2024

I have to comment out measurement_function and measurement_noise to get KalmanFilterTransformerPK to pass the check_estimator(kf.KalmanFilterTransformerPK, raise_exceptions=True) test, but I don't think it's a good idea to simply delete the two parameters, could you @fkiraly help me to resolve this?

Of course. Could you kindly explain quickly:

  • what are these parameters of?
  • why does check_estimator fail, to the best of your knowledge? E.g., test, reason for failure, parameter setting?
  • why does commenting out silence the issue? Even if it does not fix it
  • have you thought of any alternative fixes?

@XinyuWuu
Copy link
Member Author

@fkiraly Thank you for your explanation!

  • what are these parameters of?

The measurement_function is a matrix to convert the measurement (X) to state (Y) and measurement_noise is the matrix that defines the noise of the measurement.

  • why does check_estimator fail, to the best of your knowledge? E.g., test, reason for failure, parameter setting?

check_estimator fails because it have different tests with X of different shapes passed to KalmanFilterTransformerPK.

  • why does commenting out silence the issue? Even if it does not fix it

If commenting out the two parameters, the KalmanFilterTransformerPK will have default behavior based on the shape of X. If a specific measurement_function or measurement_noise with a specific shape passed to KalmanFilterTransformerPK, the filter will expect the shape of the input X to be compatible with the two parameters. However, in check_estimator, there are different tests with X of different shapes passed to KalmanFilterTransformerPK. Some X is inferring measurement_dim = 1, but other X is inferring measurement_dim = 2.

  • have you thought of any alternative fixes?
  1. We can change the behavior of check_estimator, make it pass X with a specific shape according to measurement_dim inferred by measurement_function and measurement_noise.
  2. We can comment out the two parameters, so the KalmanFilterTransformerPK will behave according to measurement_dim inferred by X during different tests in check_estimator.

@fkiraly
Copy link
Collaborator

fkiraly commented Mar 16, 2024

Ah, thanks for the explanation, I think I understand now. I am trying to rephrase to check my understanding, please let me know if this is correct or not.

The issue is coming from two variables in the new parameter set, measurement_dim and measurement_noise.

The problem is that these parameters need to be of shape compliant with the data seen by the transformer, but in the test parameter set you have to pick one value, while the check_estimator utility substitutes data of different shape, so at least one of these combinations will be non-compliant and trigger errors.

Have I understood this right?

In this case, I would say:

  • I think the variable shape parameters are already tested in test_kalman_filter separately, so we do not need to cover them here.
  • I would suggest to just rempve them from the standard tests, as you suggest.
  • Possibly one might also think about raising a useful error message, although that should be coming from the interfaced kalman filter package. Do you think the error message is clear enough for the user?

@XinyuWuu
Copy link
Member Author

Thank you for your help, I haven't noticed the test_kalman_filter unit tests. I still need more time to get familiar with pytest and the overall test structure in sktime. Using breakpoints to debug the test function which includes lots of nested loops is not a good idea. Thank you for your suggestion, removing the two parameters would be a reasonable choose. I have removed them, and now both KalmanFilterTransformerFP and KalmanFilterTransformerPK would pass the test. For the error message, I think it's clear enough. Without estimate_matrices parameter, the error message will even include suggestion of the right shape of wrong parameters.

@fkiraly
Copy link
Collaborator

fkiraly commented Mar 16, 2024

great, I'm restarting the tests then

Copy link
Collaborator

@fkiraly fkiraly 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 the added tests and the explanation!

@fkiraly fkiraly merged commit 396e85b into sktime:main Mar 16, 2024
54 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Adding new functionality module:transformations transformations module: time series transformation, feature extraction, pre-/post-processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants