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

Simplify fluid api for fit a line #10301

Merged
merged 39 commits into from
May 15, 2018
Merged

Simplify fluid api for fit a line #10301

merged 39 commits into from
May 15, 2018

Conversation

daming-lu
Copy link
Contributor

modified test_fit_a_line.py to follow the new pattern here

@CLAassistant
Copy link

CLAassistant commented Apr 30, 2018

CLA assistant check
All committers have signed the CLA.

@daming-lu daming-lu requested a review from cs2be April 30, 2018 21:01
@helinwang helinwang changed the title Simplify fluid api Simplify fluid api for fit a line May 1, 2018
@helinwang helinwang added this to In Progress in Fluid API Simplification May 1, 2018
@daming-lu daming-lu requested review from reyoung and removed request for cs2be May 5, 2018 01:01
return

place = fluid.CUDAPlace(0) if use_cuda else fluid.CPUPlace()
exe = fluid.Executor(place)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the purpose of this API Simplification effort is to hide details like Executor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. The infer is in old format for a reason. #10426

I will change it to new API once it is implemented. The current PR only changed trainer.train()

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sidgoyal78 Thanks Sid. I kept the old infer() on purpose. I want the test to be OK with new trainer.train and old infer.

@wangkuiyi
Copy link
Collaborator

@daming-lu Please take a look at #10248 for your reference. Thanks!

return avg_loss


def train(use_cuda, save_dirname, is_local):

Choose a reason for hiding this comment

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

is_local is unused. What is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Removed. Also triggered TeamCity

# NOT clear yet
# fluid.io.save_inference_model(save_dirname, ['x'], [y_predict])
# trainer.save_params(save_dirname)
trainer.save_inference_model(save_dirname)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the correct way to save model now. See here

@daming-lu
Copy link
Contributor Author

@tonyyang-svail Thanks to YangYang for the bug fix. It was a tricky one 😭

Copy link

@tonyyang-svail tonyyang-svail left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks, @daming-lu , this would be a good start to migrate to the new API.

@daming-lu daming-lu merged commit 9fad436 into PaddlePaddle:develop May 15, 2018
@jacquesqiao jacquesqiao moved this from In Progress to Done in Fluid API Simplification May 17, 2018
@daming-lu daming-lu deleted the simplify_fluid_api branch May 18, 2018 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants