Skip to content

add stacking selector functionality and docstrings - #369

Merged
solegalli merged 28 commits into
mainfrom
stack_sels
Feb 18, 2022
Merged

add stacking selector functionality and docstrings#369
solegalli merged 28 commits into
mainfrom
stack_sels

Conversation

@solegalli

Copy link
Copy Markdown
Collaborator

closes #361
closes #309

Hi @gverbock

I rebased main to solve the pytest errors and add docstring functionality and then cherry picked some of the changes in #361.

I still need to cherry-pick the test changes, and we need to add the confirm_variables functionality to the remaining selectors (except the target mean because we need to fix that in a different PR)

@solegalli

Copy link
Copy Markdown
Collaborator Author

I think I brought all the changes in PR #361 to this one now.

@gverbock gverbock mentioned this pull request Feb 1, 2022
@solegalli

Copy link
Copy Markdown
Collaborator Author

Hi @gverbock

It would be great if you can pick up this changes in a new branch and then make a PR to the branch here, stack_sels.

Also, since all classes have the functionality for confirm_variables, instead of adding 1-2 tests per class, can we add 1 generic test in the script check_estimator.py at the root of the package and then call that test from check_estimator_selectors ?

@gverbock

gverbock commented Feb 2, 2022

Copy link
Copy Markdown
Contributor

Hi @solegalli
I see that you have indeed done quite some work. It looks very good. I fetched the stack_sels branch and made a new branch out of it.
Just to be sure we are aligned before I start working on it:

  • I see that the confirm_variable is implemented for all selectors it was meant to be implemented.
  • I will pick-up the generic test on confirm_variable.
  • I am not sure if there is something left after that, what is your view on this?

@solegalli

Copy link
Copy Markdown
Collaborator Author
  • not sure if there is somethi

Hi @gverbock

I am not sure why I did not receive a notification for your last message. I am really sorry I reply only today :_(

I believe I have now added all the necessary changes. Needs a final review to merge.

@solegalli

Copy link
Copy Markdown
Collaborator Author

Still needs revision:

  • remove tests for confirm variables in individual test files (we now have a generic test in estimator_checks.py.
  • see if we can add a test in check_confirm_variables in estimator_checks.py initialising the transformers with variables=None.

I think that will be all.

@solegalli
solegalli merged commit 5d3a23c into main Feb 18, 2022
@solegalli
solegalli deleted the stack_sels branch February 18, 2022 00:37
solegalli added a commit that referenced this pull request Feb 18, 2022
* creates abstraction for confirm vars

* adds docstrings for confirm vars

* picks var manipulation and tests

* minor edits to base_selector

* adds docstring func to drop constant

* adds docstring func to drop correlated

* adds docstring func to drop duplicated

* adds docstring func to drop high psi

* adds spaces in docstrings drop features

* adds docstring funct to all selectors

* fixes most circle ci errors

* adds test confirm_vars to drop constant selector

* adds test confirm_vars to drop corr selector

* adds test confirm_vars to drop dupes selector

* adds test confirm_vars to drop high psi selector

* fixes style

* finishes pr

* tox auto fixes

* ignores flake8 in selector docstrings

* changes wording of some dosctrings

* final docstrings fixes

* small change in recursive base

* update wording confirm_variables

* removes whitespace in sel docstrings

* changes wording in selectors docstrings common file

* final wording changes, removes unused docstring

* add test confirm_variables when variables is None

* role back test files to main
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.

Adjust variables when piling up selectors

2 participants