Skip to content
This repository has been archived by the owner on Sep 18, 2024. It is now read-only.

refactor experiment summary file(to fix componentWillReceiveProps waring) and click other areas to close panel #2734

Merged
merged 10 commits into from
Aug 7, 2020

Conversation

Lijiaoa
Copy link
Contributor

@Lijiaoa Lijiaoa commented Jul 24, 2020

No description provided.

@scarlett2018 scarlett2018 mentioned this pull request Jul 24, 2020
66 tasks
// add intermediate result message
trialMessagesArr[item].intermediate = [];
Object.keys(interResultList).map(key => {
const interId = `${interResultList[key].trialJobId}-${interResultList[key].parameterId}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the id format has changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

had updated.

.then(data => this.doUpdateMetrics(data as any, false))
.then(data => {
this.metricsList = data;
this.doUpdateMetrics(data as any, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

return this.doUpdateMetrics

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why return this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok. will return this function

@@ -252,7 +262,8 @@ class TrialManager {
return updated;
}

private async updateMetrics(lastTime?: boolean): Promise<boolean> {
private async updateMetrics(lastTime?: boolean): Promise<boolean | void> {
// private async updateMetrics(lastTime?: boolean): Promise<boolean> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove unused code. Why should it be boolean | void?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

add void type to fix this error:
image
had delete line 266.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

finally, use line 265. Thanks.

@@ -58,7 +67,7 @@ class TrialManager {
}
}

public async update(lastTime?: boolean): Promise<boolean> {
public async update(lastTime?: boolean): Promise<boolean | void> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

had fixed.

{isvisibleLogDrawer && <LogDrawer closeDrawer={this.closeLogDrawer} />}
<ExperimentDrawer isVisble={isvisibleExperimentDrawer} closeExpDrawer={this.closeExpDrawer} />
{isvisibleLogDrawer && <LogPanel closeDrawer={this.closeLogDrawer} />}
{isvisibleExperimentDrawer && <ExperimentPanel closeExpDrawer={this.closeExpDrawer} experimentProfile={EXPERIMENT.profile} />}
Copy link
Contributor

@liuzhe-lz liuzhe-lz Aug 3, 2020

Choose a reason for hiding this comment

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

I don't know if it's proper to pass EXPERIMENT.profile (a getter function) as property. This will likely to cause ExperimentPanel get re-rendered every time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

had tested this status. ExperimentPanel will refresh as interval not every time.

public getMetricsList(): Array<any> {
return this.metricsList;
}
public getTrialJobList(): Array<any> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Trial job should have a type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

had fixed. add type MetricDataRecord TrialJobInfo.

componentWillUnmount(): void {
this._isCompareMount = false;
this._isExperimentMount = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Extra space before =.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@ultmaster ultmaster merged commit 2488aa6 into microsoft:master Aug 7, 2020
LovPe pushed a commit to LovPe/nni that referenced this pull request Aug 17, 2020
…ing) and click other areas to close panel (microsoft#2734)

(cherry picked from commit 2488aa6)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants