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

Rename and add member variables #197

Merged
merged 2 commits into from
Jun 1, 2023
Merged

Rename and add member variables #197

merged 2 commits into from
Jun 1, 2023

Conversation

rosecers
Copy link
Collaborator

@rosecers rosecers commented May 30, 2023

As requested, the portions of #196 that only deal with renaming member variables. I tried my best to keep it as clean as possible, but it is a large change.


📚 Documentation preview 📚: https://scikit-matter--197.org.readthedocs.build/en/197/

@rosecers rosecers force-pushed the fix/member_variables branch 4 times, most recently from 64a31ae to d5f020e Compare May 30, 2023 17:15
@rosecers rosecers force-pushed the fix/member_variables branch from d5f020e to 3ff1bab Compare May 30, 2023 17:16
@rosecers rosecers changed the title Initial commit Rename and add member variables May 30, 2023
Copy link
Collaborator

@agoscinski agoscinski 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 work, it really simplifies reviewing! There is just one comment on the axis. Otherwise it is good to go. A suggestion for the final commit message

Renaming private member variables and updating tests

* Renaming private member variables marked as private to sklearn style 
* Adding n_samples_in_, self.n_features_in_ to sklearn classes
* tests/test_feature_pcov_cur.py Adding target y to fitting for consistency
* tests/test_greedy_selector.py Removing unnecessary selector_sample fit and update error message
* tests/test_kernel_pcovr.py Adding tests  for different solvers
* tests/test_orthogonalizers.py Fix small doc typo

src/skmatter/_selection.py Outdated Show resolved Hide resolved
@rosecers rosecers force-pushed the fix/member_variables branch from 4019b35 to 1265be3 Compare June 1, 2023 01:45
@rosecers rosecers requested a review from agoscinski June 1, 2023 13:48
Copy link
Collaborator

@agoscinski agoscinski 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 work again. Looks good!

@agoscinski
Copy link
Collaborator

Suggestion for commit message

Renaming private member variables and updating tests

* Renaming private member variables marked as private to sklearn style 
* Adding n_samples_in_, self.n_features_in_ to sklearn classes
* tests/test_feature_pcov_cur.py Adding target y to fitting for consistency
* tests/test_greedy_selector.py Removing unnecessary selector_sample fit and update error message
* tests/test_kernel_pcovr.py Adding tests  for different solvers
* tests/test_orthogonalizers.py Fix small doc typo

@rosecers rosecers merged commit ec67c46 into main Jun 1, 2023
@rosecers rosecers deleted the fix/member_variables branch June 1, 2023 16:56
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.

2 participants