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

MRG: implement ResidualizeTarget #11

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

zuxfoucault
Copy link

implement ResidualizeTarget at HBM brainhack

@raamana
Copy link
Owner

raamana commented Jun 17, 2020

You also need to few tests corresponding to this class!

@raamana raamana self-requested a review June 17, 2020 04:17
@raamana
Copy link
Owner

raamana commented Jun 17, 2020

  1. undo changes to init.py

  2. test is not correct, you ensure deconfounded targets (not X) are orthogonal to the confounds


in_targets = check_array(in_targets)
confounds = check_array(confounds, ensure_2d=False)

Copy link
Owner

Choose a reason for hiding this comment

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

Add checks for data type to ensure it is numeric


check_is_fitted(self, 'model_', 'n_targets_')
test_targets = check_array(test_targets, accept_sparse=True)

Copy link
Owner

Choose a reason for hiding this comment

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

add another data type check

Parameters
----------
X : {array-like, sparse matrix}, shape (n_samples, n_targets)
The training input samples.
Copy link
Owner

Choose a reason for hiding this comment

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

make it clear X is targets here, although it is called X

max_dim = 100
for n_samples in np.random.randint(0, 20, 1):
for num_confounds in np.random.randint(min_dim, max_dim, 3):
train_all, _ = make_sparse_uncorrelated(
Copy link
Owner

Choose a reason for hiding this comment

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

this does not generate discrete numerical/integer values! We need a way to broaden the test with multiple data types!

Copy link
Author

Choose a reason for hiding this comment

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

a bit confused here, does this look better? np.random.randint(0, 20, 1, dtype=int)

Copy link
Owner

Choose a reason for hiding this comment

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

before updating the tests, how about you try making a jupyter notebook, and play with this class, and write some examples, to see how it works?

Copy link
Author

Choose a reason for hiding this comment

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

hey @raamana sorry for the late response. this summer has been a bit crazy. probably need more time-out here. i noticed that some repos have been snapshotted in Arctic Code Vault, i guess it didn't snapshot PR :p

Copy link
Owner

Choose a reason for hiding this comment

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

NP, take your time. Happy to chat with you to discuss this and make a more precise plan to finish it quickly.

why are you referencing the Arctic Code Vault? did you lose your fork or what?

Copy link
Author

Choose a reason for hiding this comment

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

thanks! i'm wondering if the snapshot in Arctic Code Vault would include PR? and no, i didn't lose forks.

Copy link
Owner

Choose a reason for hiding this comment

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

I don't know - but I am sure your contribution will be clearly recognized :)

Copy link
Owner

Choose a reason for hiding this comment

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

hi @zuxfoucault, would you have time this year to work on this?

Copy link
Author

Choose a reason for hiding this comment

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

Hi, sorry I just saw this.

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.

None yet

2 participants