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

Dev sdp parallel #102

Closed
wants to merge 23 commits into from
Closed

Dev sdp parallel #102

wants to merge 23 commits into from

Conversation

trigaut
Copy link
Member

@trigaut trigaut commented Jun 22, 2016

closes #75 with PR #100
Add working implementation of parallelized SDP using multiprocessing

small benchmark:

  • the computation might be a bit slower on very small instances
  • the computational gain in the other cases (discretized state and control spaces size > 100) is between 30 to 50% with 2 processes
  • there is a small computational gain even with 1 process for "regular" and large instances

TODO: update doc with frapac help


- v (Array)
the value function to interpolate
- dim_states (Int)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should use the markdown everywhere :)

@leclere
Copy link
Collaborator

leclere commented Jun 23, 2016

I do not understand what is the type of V now ?

import StochDynamicProgramming, Distributions
println("library loaded")

@everywhere begin
Copy link
Collaborator

Choose a reason for hiding this comment

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

Explain this line

Copy link
Member Author

Choose a reason for hiding this comment

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

we have to define the instance across all workers if we parallelize. The user won't have to do it if we develop an interface with macros

Copy link
Member Author

Choose a reason for hiding this comment

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

To answer the question above regarding the type of V (I cannot answer directly below I don't know why maybe you commented the PR then commented a diff after). V is now a SharedArray because it is shared by multiple processes

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, then add a comment explaining the line. Otherwise user won't know if they need to copy it, modify it etc...

Same for V, at least in the comments above the function you should say that V is an array or a sharedArray of this or that form.

@trigaut
Copy link
Member Author

trigaut commented Jun 27, 2016

I made the changes you required. Frapac is benchmarking to conclude if we have to switch to the old implementation of SDP if it is not parallelized. I don't think we should because the new implementation is slower only on really small instances but faster of others even if it is not parallelized.

- V (Array of SharedArray)
the array containing the discretized value functions at each time step

- Vitp (Interpolations)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is "Interpolations" ?

@trigaut
Copy link
Member Author

trigaut commented Jun 29, 2016

I updated the battery example. It is more correct now and I think clearer. Thanks to frapac benchmark it seems that it is better to keep this implementation and not make a switch so after we have convert the comments to markdown I think we can merge. Unless you have other comments of course

@trigaut
Copy link
Member Author

trigaut commented Jul 12, 2016

@frapac can you use your script to convert the docstrings to markdown please? After that I think this PR can be merged

@frapac frapac closed this Jul 18, 2016
@frapac frapac deleted the dev_sdp_parallel branch July 18, 2016 12:03
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.

4 participants