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

Performance Tweaks #55 #56

Merged
merged 21 commits into from Jan 18, 2019

Conversation

Projects
None yet
2 participants
@codingdna2
Copy link

codingdna2 commented Jan 10, 2019

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

Danilo Corallo added some commits Jan 10, 2019

Danilo Corallo
Performance Optimizations issue #55
Performance Optimizations issue #55
Danilo Corallo
Danilo Corallo

@codingdna2 codingdna2 changed the title Performance Optimizations issue #55 Performance Tweaks #55 Jan 11, 2019

@giacomelli giacomelli self-assigned this Jan 14, 2019

@giacomelli
Copy link
Owner

giacomelli left a comment

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

This comment has been minimized.

Copy link
Author

codingdna2 commented Jan 16, 2019

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

@codingdna2

This comment has been minimized.

Copy link
Author

codingdna2 commented Jan 16, 2019

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

@giacomelli

This comment has been minimized.

Copy link
Owner

giacomelli commented Jan 16, 2019

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
Copy link
Author

codingdna2 left a comment

Please comment about the change in DefaultOperatorsStrategy.cs:

  • Changed loop use parents.Count, previously was using population. Did it to save from passing population.
D
Fixed issue with TplTaskExecutor, added speed in console app, applied…
… TplTaskExecutor to Ghostwriter as example
D
@codingdna2

This comment has been minimized.

Copy link
Author

codingdna2 commented Jan 16, 2019

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

This comment has been minimized.

Copy link
Owner

giacomelli commented Jan 17, 2019

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

This comment has been minimized.

Copy link
Author

codingdna2 commented Jan 18, 2019

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

Danilo added some commits Jan 18, 2019

Danilo
Revert "Simple fix proposal for PopulationTest.CreateInitialGeneratio…
…n_AdamChromosomeCreateNewNull_Exception"

This reverts commit 0dd670d.
Danilo
@codingdna2

This comment has been minimized.

Copy link
Author

codingdna2 commented Jan 18, 2019

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

Danilo
@codingdna2

This comment has been minimized.

Copy link
Author

codingdna2 commented Jan 18, 2019

My bad, it's now fixed.

@giacomelli giacomelli changed the base branch from master to develop Jan 18, 2019

@giacomelli

This comment has been minimized.

Copy link
Owner

giacomelli commented Jan 18, 2019

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

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment