-
Notifications
You must be signed in to change notification settings - Fork 133
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
Add parameter to allow equal weight during HPAL level 1 iteration. #532
Conversation
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #532 +/- ##
==========================================
- Coverage 76.66% 75.63% -1.04%
==========================================
Files 364 363 -1
Lines 41341 41370 +29
Branches 6667 6672 +5
==========================================
- Hits 31695 31289 -406
- Misses 7751 8231 +480
+ Partials 1895 1850 -45
Continue to review full report at Codecov.
|
Could you add some unit test code to test the new feature please? |
ha = Hyperalignment(ref_ds=ref_ds, | ||
zscore_all=zscore_all, | ||
zscore_common=zscore_common) | ||
zscore_common=zscore_common, | ||
level1_equal_weight=level1_equal_weight) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is a start! ;) although too expensive one I must say, but how do we know that the "logic" is actually working? ;-)
@feilong could you please add a simple test for mean_xy
which would demonstrate that if we provide similar to the ones used in the code weighting, it would produce the desired equivalently balanced contribution?
I know that it sounds "not that interesting", but that is why they are called "unit-tests".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem, just added a new commit with the tests for mean_xy
.
|
@@ -430,7 +437,12 @@ def _level1(self, datasets, commonspace, ref_ds, mappers, residuals): | |||
# to make a batch update after processing all 1st-level datasets | |||
# to an identical 1st-level common space | |||
# TODO: make just a function so we dont' waste space | |||
commonspace = params.combiner1(ds_, commonspace) | |||
if params.level1_equal_weight: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the TODO still applicable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
who knows... probably still is (for "inplace" zscoring, right @swaroopgj?), but it is unrelated to this PR
yeap, not your fault... for now just restarted that run, let's pretend it didn't happen for now ;-) |
Seems to be LGTM, will merge |
I raised an issue, let's keep an eye on it. |
If this looks good to you I'll also update
searchlight_hyperalignment.py
to make similar changes to SL HA. @yarikoptic @swaroopgj