Complexity property test #74
Comments
|
Let's discuss it in a new issue. Maybe the default values for the regularization parameters are not comparable enough, or we should average over more datasets to account for the randomness. |
|
I like the idea of averaging over multiple datasets. So the idea would be that we would have the function, say, look over some nearby seed values, compute the average complexity for each optimizer across the different resulting datasets, then do the comparison? |
|
Okay, I created a quick PR #75 implementing the averaging idea. Let me know what you think. |
jpcurbelo
pushed a commit
to jpcurbelo/pysindy_fork
that referenced
this issue
May 9, 2024
This commit unifies the way the different models preprocess the static and dynamic input data. Closes dynamicslab#57 and dynamicslab#74, part of dynamicslab#64. Similar to the model head, there is now an InputLayer that all (single-frequency) models use. Depending on the config, this layer will: - pass the static variables through an embedding layer (if cfg.statics_embedding is True) - pass the dynamic variables through an embedding layer (if cfg.dynamics_embedding is True) - concatenate static to each step of dynamic inputs (unless this is deactivated; e.g., for EA-LSTM this is not wanted) Key changes: - New config arguments statics_embedding, dynamics_embedding (both bool, default False). - EmbCudaLSTM is no longer needed since the functionality is now part of CudaLSTM. For backwards compatibility, the class still exists but defers to CudaLSTM. - If EA-LSTM and static embeddings are used, there will now be a linear layer between the embedding output and the lstm input to make sure the dimensions match. Previously, the linear layer would be left out if static embeddings were used.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Sorry, I didn't see your comment before making the most recent commit. I relaxed the complexity test constraint a bit more.
I was able to reproduce the failed test locally by using the same parameters:
n_samples=1124,n_features=27,n_informative=5,random_state=6. It looks like LASSO somehow finds 4 informative features, while the other heavily regularized models find 6. This seems like a bit of a fluke; for different random states this doesn't occur.Maybe we could remove LASSO from this test? Or we could stick with the quick fix I made.
In any case, the bug isn't caused by this PR, so I think it's okay to accept it.
Originally posted by @briandesilva in #71 (comment)
The text was updated successfully, but these errors were encountered: