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

Sidewalk alignment #189

Closed
wants to merge 17 commits into from
Closed

Sidewalk alignment #189

wants to merge 17 commits into from

Conversation

@benjello
Copy link
Collaborator

benjello commented Oct 28, 2015

I resubmit a PR similar to #188 after taking care of some of the issues raised by @gdementen
Remaining potential problems are:

  • Didn't make it to run the tests without method = method kwarg
  • Left a commented line in code that might be a fix to future problems IIRC
  • Commented out tests that I do not know if they can be applied to the sidewalk method (let me know how i can adjust them or if i should delete them)
  • Removed simple2001.h5 and small.h5 from the repo according to .gitignore
@landscape-bot
Copy link

landscape-bot commented Oct 28, 2015

Code Health
Repository health decreased by 0.02% when pulling b5e50dd on benjello:sidewalk_alignment into 7709f8f on liam2:master.

@landscape-bot
Copy link

landscape-bot commented Oct 28, 2015

Code Health
Repository health increased by 0.00% when pulling 09fe8f4 on benjello:sidewalk_alignment into 7709f8f on liam2:master.

sorted_local_indices = np.argsort(maybe_members_rank_value)
sorted_global_indices = \
group_maybe_indices[sorted_local_indices]
if method == 'sidewalk':

This comment has been minimized.

# take the last X individuals (ie those with the highest score)
indices_to_take = sorted_global_indices[-maybe_to_take:]
elif method == 'sidewalk':
assert maybe_to_take <= sum(score[sorted_global_indices]), \

This comment has been minimized.

@gdementen

gdementen Oct 28, 2015 Member

shouldn't use an assert for user errors (raise a ValueError in this case)

@@ -180,7 +203,8 @@ def collect_variables(self):
return set()

def _eval_need(self, context, need, expressions, possible_values,
expressions_context=None):
method, expressions_context=None):

This comment has been minimized.

@gdementen

gdementen Oct 28, 2015 Member

Why do you need the new argument? It doesn't seem to be used!

@@ -329,15 +368,17 @@ def compute(self, context, score, need, filter=None, take=None, leave=None,

func = self.align_no_link if link is None else self.align_link
return func(context, score, need, filter, take, leave, expressions,
possible_values, errors, frac_need, link, secondary_axis)
possible_values, errors, frac_need, link, secondary_axis, method=method)

This comment has been minimized.

@gdementen

gdementen Oct 28, 2015 Member

The rule I use is: if I know I use all arguments in all usages of the function, I don't name arguments explicitly.

@landscape-bot
Copy link

landscape-bot commented Oct 28, 2015

Code Health
Repository health increased by 0.00% when pulling a2efb63 on benjello:sidewalk_alignment into 7709f8f on liam2:master.

@gdementen
Copy link
Member

gdementen commented Oct 28, 2015

In its current form, the need is not really respected/does not really makes sense (AFAICT). However, it should be possible to simply scale the score/probabilities to achieve the "needed" figures, even if need is N dimensional.

"arguments")
elif method == 'sidewalk':
raise Exception("If alignment on sum of score is wanted "
"then use align_abs")

This comment has been minimized.

@gdementen

gdementen Oct 28, 2015 Member

It took me several reads to understand what this means. Any idea to make this clearer?

@benjello benjello force-pushed the benjello:sidewalk_alignment branch from a2efb63 to cfafa58 Oct 29, 2015
@landscape-bot
Copy link

landscape-bot commented Oct 29, 2015

Code Health
Repository health increased by 0.00% when pulling cfafa58 on benjello:sidewalk_alignment into 7344808 on liam2:master.

@gdementen
Copy link
Member

gdementen commented Oct 31, 2015

removing small.h5 is a bit harsh. It either needs to stay (but please do not commit it when it's changed), OR run import.yml before you run simulation.yml (and the variants which also use small.h5)

@landscape-bot
Copy link

landscape-bot commented Nov 2, 2015

Code Health
Repository health increased by 0.00% when pulling ac3cbdc on benjello:sidewalk_alignment into 7344808 on liam2:master.

@benjello benjello force-pushed the benjello:sidewalk_alignment branch from ac3cbdc to 8537823 Nov 2, 2015
@landscape-bot
Copy link

landscape-bot commented Nov 2, 2015

Code Health
Repository health increased by 0.00% when pulling c3498be on benjello:sidewalk_alignment into d44f585 on liam2:master.

@gdementen

This comment has been minimized.

Copy link

gdementen commented on liam2/alignment.py in c3498be Nov 2, 2015

I would have gone for something like "sidewalk method is not supported for align(), please use align_abs() instead", what do you think? If you think your version is better, please fix the typo in aligned :)

@@ -179,8 +202,8 @@ def collect_variables(self):
# in this case, it's tricky
return set()

def _eval_need(self, context, need, expressions, possible_values,
expressions_context=None):
def _eval_need(self, context, need, expressions, possible_values, expressions_context=None):

This comment has been minimized.

@gdementen

gdementen Nov 2, 2015 Member

could you revert this line change?

@gdementen
Copy link
Member

gdementen commented Nov 2, 2015

This looks mostly good to me. I just wonder what is wrong about the test using values from .csv files?

Anyway, I will not have time for a in-depth review & merge for a couple of weeks. As for the default values PR, could you provide documentation and add a mention in the changelog at doc/usersguide/source/changes/version_0_11.rst.inc
Thanks!

@landscape-bot
Copy link

landscape-bot commented Nov 2, 2015

Code Health
Repository health increased by 0.00% when pulling 0348f43 on benjello:sidewalk_alignment into d44f585 on liam2:master.

@landscape-bot
Copy link

landscape-bot commented Nov 12, 2015

Code Health
Repository health increased by 0.00% when pulling c2ad9ba on benjello:sidewalk_alignment into 0e675a4 on liam2:master.

@landscape-bot
Copy link

landscape-bot commented Nov 12, 2015

Code Health
Repository health increased by 0.00% when pulling 569d40c on benjello:sidewalk_alignment into 58bea98 on liam2:master.

@landscape-bot
Copy link

landscape-bot commented Nov 16, 2015

Code Health
Repository health increased by 0.00% when pulling 8fae64d on benjello:sidewalk_alignment into bfcae0a on liam2:master.

@gdementen gdementen self-assigned this Nov 20, 2015
@gdementen gdementen added this to the 0.11 milestone Nov 20, 2015
@gdementen
Copy link
Member

gdementen commented Nov 20, 2015

Merged! Thanks a lot!

@gdementen gdementen closed this Nov 20, 2015
@benjello benjello deleted the benjello:sidewalk_alignment branch Nov 24, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.