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

Improve #1081 and Refine parallel TPE's code #1296

Closed
wants to merge 65 commits into from

Conversation

xuehui1991
Copy link
Contributor

@xuehui1991 xuehui1991 commented Jul 12, 2019

Improve this PR: https://github.com/microsoft/nni/pull/1081/files

  • fix random state bug

  • refine code logic

  • refine doc and experiment log

Will update the doc and experiment log after PR #1242 merge.
Original TPE: http://172.23.234.85:9887
image

Updated code: http://172.23.234.85:9595
image

The trial code from @Crysple .

@QuanluZhang
Copy link
Contributor

@PurityFan please help review this pr.

Copy link
Contributor

@PurityFan PurityFan left a comment

Choose a reason for hiding this comment

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

Have you find the reason or solution of strange phenomenon raised by zejun?

if self.parallel and len(self.total_data)>20 and len(self.running_data):
self.CL_rval = copy.deepcopy(self.rval)
_constant_liar_y = 0
if self.constant_liar_type == 'mean' and self.optimal_y[1]:
Copy link
Contributor

Choose a reason for hiding this comment

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

could you make sure self.optimal_y must be an array here? and if self.optimal_y[1] is 0, assigning self.optimal_y to _constant_liar_y is not correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think in the original design :
when constant_liar_type == 'mean' --> self.optimal_y is a array
otherwise --> self.optimal is a value.
In one experiment only has one constant_liar_type.
@QuanluZhang

Copy link
Contributor

Choose a reason for hiding this comment

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

I added two more comments which are the reason that I have concern here.

Copy link
Contributor

Choose a reason for hiding this comment

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

discussed with @PurityFan , to fix this problem, you can simply add self.optimal_y is not None in line 422

Copy link
Contributor

Choose a reason for hiding this comment

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

can remove self.optimal_y[1] from line 424

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@QuanluZhang I don't think we could remove sel.optimal_y[1] in line 424 because it will avoid /0 in line 425.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@QuanluZhang Other comments fix in new PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

@xuehui1991 if self.optimal_y is not None, self.optimal_y[1] must be larger than 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@QuanluZhang changed. Please review new PR.

@QuanluZhang QuanluZhang self-requested a review July 26, 2019 06:19
@QuanluZhang
Copy link
Contributor

closed. this pr is replaced with another pr #1373

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

4 participants