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.

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

Maygen #811

Merged
merged 25 commits into from
Feb 6, 2022
Merged

Maygen #811

Changes from 1 commit
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
c1d87ed
Integrating MAYGEN project to structgen package
MehmetAzizYirik Nov 28, 2021
6bbfe59
Added the JavaDoc to the source and test classes.
MehmetAzizYirik Nov 30, 2021
be4d86e
Added dependencies, formatted sources.
javadev Dec 1, 2021
63b4b42
Fix up some dependencies lost in the rebase. We should not be calling…
johnmay Jan 31, 2022
6b75b18
Split out the command line parsing from the main entry point. For now…
johnmay Jan 31, 2022
be00615
Pass in the IChemObjectBuilder
johnmay Jan 31, 2022
706b8f0
Fix copyright header
johnmay Jan 31, 2022
158a34d
Remove commons-lang3, a specicalled MF range parse would be better bu…
johnmay Jan 31, 2022
48da99f
Avoid reflective calls.
johnmay Jan 31, 2022
37e1457
removeHydrogens creates a copy of the molecule, use suppressHydrogens…
johnmay Feb 1, 2022
a0dfd2b
Replace seperate output modes with a single "emit" which will consume…
johnmay Feb 1, 2022
01706c9
Remove redundant ForkJoinPool, submitting and then waiting is just ru…
johnmay Feb 1, 2022
2c4b026
May SonarCloud happier by not using a for-loop here?
johnmay Feb 1, 2022
a9d8dd3
Parameterized tests
johnmay Feb 1, 2022
2141013
Split out IO concerns from the generator, using a consumer.
johnmay Feb 5, 2022
3ccd84d
Forgot to remove the SmilesGenerator from here.
johnmay Feb 5, 2022
37b1815
Test IO generation, this is a little tricky since for example the coo…
johnmay Feb 5, 2022
4c9bb43
Add back in the brittle ForkJoin hack.
johnmay Feb 5, 2022
668a69f
SonarCloud: Use the logger for verbose output. If needed the format c…
johnmay Feb 5, 2022
ab90f91
Relicense header as LGPL to be inline with rest of CDK
johnmay Feb 5, 2022
dfa4755
Document usage of the class.
johnmay Feb 5, 2022
1d2b09d
SonarCloud suggestion, move these tests into the Parameterized ones.
johnmay Feb 5, 2022
69ccdb7
Additional SonarCloud suggestions.
johnmay Feb 5, 2022
601f02f
Added support for the smiles and sdf generator.
javadev Feb 6, 2022
eeefebd
Merge pull request #818 from jscrdev/maygen
johnmay Feb 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 2 additions & 19 deletions tool/structgen/src/main/java/org/openscience/cdk/structgen/maygen/Maygen.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,7 @@ and associated documentation files (the "Software"), to deal in the Software wit
import java.util.Map.Entry;
import java.util.Objects;
import java.util.Set;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.ForkJoinPool;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.logging.Level;
import java.util.logging.Logger;

import org.openscience.cdk.exception.CDKException;
import org.openscience.cdk.group.Permutation;
Expand Down Expand Up @@ -2594,21 +2590,8 @@ public void structureGenerator(String localFormula) {
List<int[]> newDegrees = distributeHydrogens();

if (multiThread) {
try {
new ForkJoinPool(size)
Copy link
Contributor

Choose a reason for hiding this comment

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

size is important, it can't be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Please explain, I asked @MehmetAzizYirik. My understanding this code is not doing anything since the parellel stream will not use the forjoin pool

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay see here it really does work that way but is quite opaque/brittle - https://stackoverflow.com/questions/30802463/how-many-threads-are-spawned-in-parallelstream-in-java-8. "It should be noted that the Stream APIs use of ForkJoinPool is an implementation detail. Thus, both solutions work with Oracle’s current implementation but are not guaranteed to work everywhere."

Copy link
Member Author

Choose a reason for hiding this comment

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

Why size and not -

ForkJoinPool pool = new ForkJoinPool(Runtime.getRuntime().availableProcessors());

Copy link
Member Author

@johnmay johnmay Feb 2, 2022 •
edited

Choose a reason for hiding this comment

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

And that is the default anyways for ForkJoinPool in ParallelStream which is cleaner IMO.

Copy link
Contributor

Choose a reason for hiding this comment

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

@johnmay we had that conversation with Valentyn about the size parameter, but I believe this needs to be changed like you explained we can use that other option you recommended.

Also, this parallelization actually needs to be enhanced but further tasks not for now. Thus, we dont need to keep it with the ForkJoinPool but can change it later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please explain, I asked @MehmetAzizYirik. My understanding this code is not doing anything since the parellel stream will not use the forjoin pool

It is related for the bug MehmetAzizYirik/MAYGEN#48

In some configuration default value is 79. We need to control it not use default.

.submit(
() ->
newDegrees
.parallelStream()
.forEach(new Generation(this)::run))
.get();
} catch (InterruptedException | ExecutionException ex) {
if (verbose) {
Logger.getLogger(Maygen.class.getName())
.log(Level.SEVERE, ex, () -> "Formula " + localFormula);
}
Thread.currentThread().interrupt();
}
newDegrees.parallelStream()
.forEach(new Generation(this)::run);
} else {
newDegrees.forEach(new Generation(this)::run);
}
Expand Down