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

Bad code duplication in experiment.py #81

Closed
gbordyugov opened this issue Feb 27, 2017 · 8 comments
Closed

Bad code duplication in experiment.py #81

gbordyugov opened this issue Feb 27, 2017 · 8 comments

Comments

@gbordyugov
Copy link
Contributor

gbordyugov commented Feb 27, 2017

Experiment.delta() is now a dispatcher, however those lines https://github.com/zalando/expan/blob/dev/expan/core/experiment.py#L368 duplicate the code https://github.com/zalando/expan/blob/dev/expan/core/experiment.py#L455

Additional complication: delta() function calls the other one, i.e. fixed_horizon_delta()

@chaitanyasd
Copy link

I'm a newbie. Can i take this? Please can you help.

@jbao
Copy link

jbao commented Feb 28, 2017

@csdeshpande19 sure, this requires some de-duplication of the delta() and fixed_horizon_delta() function. Let us know if you have any specific questions.

@chaitanyasd
Copy link

Can you tell me what exactly I have to fix so i can start working on it.

@jbao
Copy link

jbao commented Feb 28, 2017

My idea would be to remove the duplicated code in fixed_horizon_delta() and do all the pre-processing in delta(), but feel free to open a PR and maybe it would become easier to discuss. What do you think?

@chaitanyasd
Copy link

Yeah. No problem. I'll do a PR in few hours as I'm new to Open Source so it may take some time to get acquainted with it. I'll try to help as much as I can.

@s4826
Copy link
Contributor

s4826 commented Mar 13, 2017

Does this still need to be done?

@jbao
Copy link

jbao commented Mar 13, 2017

Hi @s4826 , yes, this one is still open AFAIK.

@s4826
Copy link
Contributor

s4826 commented Mar 13, 2017

Ok, I'll open a pull request.

@jbao jbao mentioned this issue Mar 15, 2017
Merged
@jbao jbao closed this as completed Mar 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants