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.

Sign up for GitHub

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

Jump to bottom

cross validated results #53

Merged
merged 21 commits into from
Jun 6, 2024
Merged

cross validated results #53

merged 21 commits into from
Jun 6, 2024

Conversation

YAY-C
Copy link
Contributor

@YAY-C YAY-C commented May 23, 2024 •
edited
Loading

  • added save-file for results; added sub-selection parameters
  • added cosine kernel
  • added score (r2) evaluation (maybe not needed since individual predictions are saved)
  • add explicit random-state to regression
  • What about the --debug parameter ?
  • add KWARG to save predictions for each seed
  • add test for regression, hyperparameters

@YAY-C
Copy link
Contributor Author

YAY-C commented May 23, 2024 •
edited
Loading

I realize now that we have no tests for the regression/prediction scripts!
@briling how would you test them ? on a toy-example or a small dataset included as example ?

YAY-C
YAY-C commented May 23, 2024
View reviewed changes
qstack/regression/cross_validate_results.py Outdated Show resolved Hide resolved
qstack/regression/cross_validate_results.py Outdated Show resolved Hide resolved
qstack/regression/cross_validate_results.py Outdated Show resolved Hide resolved
qstack/regression/cross_validate_results.py Outdated Show resolved Hide resolved
qstack/regression/cross_validate_results.py Outdated Show resolved Hide resolved
qstack/regression/hyperparameters.py Outdated Show resolved Hide resolved
qstack/regression/regression.py Outdated Show resolved Hide resolved
qstack/regression/regression.py Show resolved Hide resolved
qstack/regression/regression.py Outdated Show resolved Hide resolved
qstack/regression/regression.py Outdated Show resolved Hide resolved
@YAY-C YAY-C requested a review from briling May 27, 2024 11:51
@YAY-C YAY-C marked this pull request as ready for review May 27, 2024 11:52
@briling
Copy link
Contributor

briling commented May 27, 2024

how would you test them ? on a toy-example or a small dataset included as example ?

just give them any X and Y files. We have something in https://github.com/lcmd-epfl/spahm and https://github.com/lcmd-epfl/spahm-rho

briling
briling reviewed May 28, 2024
View reviewed changes
qstack/regression/cross_validate_results.py Outdated Show resolved Hide resolved
qstack/regression/hyperparameters.py Outdated Show resolved Hide resolved
qstack/regression/regression.py Outdated Show resolved Hide resolved
YAY-C
YAY-C commented May 28, 2024
View reviewed changes
Copy link
Contributor Author

@YAY-C YAY-C left a comment •
edited
Loading

Choose a reason for hiding this comment

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

  • add a seed0 parameter to control the content of the array seeds

qstack/regression/hyperparameters.py Outdated Show resolved Hide resolved
qstack/regression/cross_validate_results.py Outdated Show resolved Hide resolved
@YAY-C YAY-C requested a review from briling May 28, 2024 15:17
@briling
Copy link
Contributor

briling commented May 28, 2024

do we need the xyz files?

@YAY-C
Copy link
Contributor Author

YAY-C commented May 29, 2024 •
edited
Loading

do we need the xyz files?

Not necessarily, but I thought we might need them for upcoming tests.
Do you think it makes more sense to remove them ?

@briling
Copy link
Contributor

briling commented May 30, 2024

do we need the xyz files?

Not necessarily, but I thought we might need them for upcoming tests. Do you think it makes more sense to remove them ?

Could you please add a test that generates the representation (used in this test) from these xyz files? so we remember they're related

@YAY-C
Copy link
Contributor Author

YAY-C commented Jun 3, 2024 •
edited
Loading

do we need the xyz files?

Not necessarily, but I thought we might need them for upcoming tests. Do you think it makes more sense to remove them ?

Could you please add a test that generates the representation (used in this test) from these xyz files? so we remember they're related

  • add test-data generation script

YAY-C
YAY-C commented Jun 5, 2024
View reviewed changes
Copy link
Contributor Author

@YAY-C YAY-C left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Do you think we need a CLI arg for the selection thing ?

I will also add the docs so it's done!

  • add documentation

YAY-C and others added 4 commits June 6, 2024 11:46
@YAY-C
Add docs and remove unecessary argument
7201479
@briling
Cross validated results (#57)
449ffea 
* Add sparse regression test

* Continue ca4dca0: add `idx_train` and refactor

* Fix CLI arguments and add docs

* Add sparse regression to all the tools
@YAY-C
Fix bug introdcued by b30a6e0 (reshaped array)
1e12849 
And removed output filename renaming (flawed)
@YAY-C
Add test for cross_validate_results.py
9489912
@briling briling merged commit d1eb9af into master Jun 6, 2024
2 checks passed
@briling briling deleted the cross-validated-results branch June 6, 2024 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@briling briling Awaiting requested review from briling

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@YAY-C @briling