Skip to content

Performance Tweaks #55 #56

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 21 commits into from
Jan 18, 2019
Merged

Performance Tweaks #55 #56

merged 21 commits into from
Jan 18, 2019

Conversation

codingdna2
Copy link

@codingdna2 codingdna2 commented Jan 10, 2019 •
edited
Loading

Performance Tweaks (PLINQ/TPL) to Genetic Algorithm #55

@codingdna2 codingdna2 changed the title Performance Optimizations issue #55 Performance Tweaks #55 Jan 11, 2019
@giacomelli giacomelli self-assigned this Jan 14, 2019
@giacomelli giacomelli mentioned this pull request Jan 14, 2019
Closed
giacomelli
giacomelli requested changes Jan 16, 2019
View reviewed changes
Copy link
Owner

@giacomelli giacomelli left a comment

Choose a reason for hiding this comment

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

Hi, the implementation it's ok as we discussed before, please, just fix the issues list below:

Remove the "OperatorsStrategies" namespace

There is no need to put the IOperatorsStrategy and its implementation in a new namespace. Please, just move them to "GeneticSharp.Domain" namespace.

Code documentation

Please, add code documentation to public members of IOperatorsStrategy and its implementations explaining their propose and use.

The same applies to all new Tpl classes.

TplPopulation

The difference between Population and TplPopulation is only the "CreateInitialGeneration" method?

Whether this the case, please, change TplPopulation to inherit from Population class, just override the "CreateInitialGeneration".

TplTaskExecutor

The difference between ParallelTaskExecutor and TplTaskExecutor is only the "Start" method?

Whether this the case, please, change TplTaskExecutor to inherit from ParallelTaskExecutor class, just override the "Start".

Unit tests

Add unit tests validating the new Tpl classes and the IOperatorsStrategy's implementations.

Solve issues

Please, solve the issues listed on https://sonarcloud.io/project/issues?branch=feature%2Fperformance-tweaks&id=GeneticSharp&resolved=false

Next steps

Feel free to question about any of the sections I pointed above.

After you finish the changes, I will perform another review.

@codingdna2
Copy link
Author

I'm pushing the first part, just wait to review it again, I still need to fix documentation and tests

@codingdna2
Copy link
Author

How can I force a SonarCloud build? I would like to check the results of my modifications...

@giacomelli
Copy link
Owner

How can I force a SonarCloud build? I would like to check the results of my modifications...

Sonar can be run from tools/runSonar.sh (or .cmd), but it's need a login from https://sonarcloud.io, in this case is my account. You can create a https://sonarcloud.io account for you and change runSonar.sh|cmd to your account details.

Anyway, I ran it again from your latest code: https://sonarcloud.io/project/issues?branch=feature%2Fperformance-tweaks&id=GeneticSharp&resolved=false

codingdna2
codingdna2 commented Jan 16, 2019
View reviewed changes
Copy link
Author

@codingdna2 codingdna2 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Please comment about the change in DefaultOperatorsStrategy.cs:

  • Changed loop use parents.Count, previously was using population. Did it to save from passing population.

Fixed issue with TplTaskExecutor, added speed in console app, applied…
e24a7ff 
… TplTaskExecutor to Ghostwriter as example
@codingdna2
Copy link
Author

codingdna2 commented Jan 16, 2019 •
edited
Loading

I'm pushing the second part, still wait to review it, I still need to fix documentation and tests.

Open Questions:

  1. Please review comment in DefaultOperatorsStrategy.cs

  2. I guess I should make the OperatorsStrategy part of the ISampleController in the GeneticSharp.Runner.ConsoleApp. Right?

  3. Where is the wiki documentation? Is not included in the project?

  4. Are you OK to use Ghostwriter as sample for Tpl classes or you prefer to leave the samples untouched?

PS: Can you run the sonarcloud build once again?

@giacomelli
Copy link
Owner

giacomelli commented Jan 17, 2019 •
edited
Loading

I'm pushing the second part, still wait to review it, I still need to fix documentation and tests.

Open Questions:

  1. Please review comment in DefaultOperatorsStrategy.cs
  2. I guess I should make the OperatorsStrategy part of the ISampleController in the GeneticSharp.Runner.ConsoleApp. Right?
  3. Where is the wiki documentation? Is not included in the project?
  4. Are you OK to use Ghostwriter as sample for Tpl classes or you prefer to leave the samples untouched?

PS: Can you run the sonarcloud build once again?

  1. Reviewed.

  2. Maybe there are no need, because any sample can change it on ConfigGA method.

  3. The wiki is a separated repo (https://github.com/giacomelli/GeneticSharp.wiki.git), but to keep things simple, you can change it directly on https://github.com/giacomelli/GeneticSharp/wiki.

  4. It's better to keep the already existing samples untouched, but you can create a new one if you have any idea.

I ran SonarCloud again.

@codingdna2
Copy link
Author

I've completed the required changes. You can now proceed to review the PR. Thanks, D.

giacomelli
giacomelli requested changes Jan 18, 2019
View reviewed changes
Copy link
Owner

@giacomelli giacomelli left a comment

Choose a reason for hiding this comment

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

I leave some comments in the code and you need to improve the TpTaskExecutor test coverage:
https://sonarcloud.io/component_measures?branch=feature%2Fperformance-tweaks&id=GeneticSharp&metric=coverage&selected=GeneticSharp%3AGeneticSharp%3A1498D792-D02B-4A9E-8AFC-24AEBA0DEE54%3AThreading%2FTplTaskExecutor.cs

@codingdna2
Copy link
Author

I think I've completed. Can you run the check on sonarcloud one more time? Thanks

giacomelli
giacomelli requested changes Jan 18, 2019
View reviewed changes
@codingdna2
Copy link
Author

My bad, it's now fixed.

@giacomelli giacomelli changed the base branch from master to develop January 18, 2019 18:33
@giacomelli
Copy link
Owner

Thanks for the PR. I will merge it to develop branch. 👍

I need to finish some feature branches from my side, then I'll publish a new release and a nuget package with your contributions.

@giacomelli giacomelli merged commit 0f1e64e into giacomelli:develop Jan 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@giacomelli giacomelli giacomelli requested changes

Assignees

@giacomelli giacomelli

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@codingdna2 @giacomelli