new MolecularFormulaGenerator #103
Conversation
| * </pre> | ||
| * | ||
| * The code was originally developed for a MZmine 2 framework module, published | ||
| * in Pluskal et al., Anal Chem 2012. |
There was a problem hiding this comment.
Here you should use the {@cdk.cite ...} approach. This works pretty much like @cite{} in LaTeX and the matching "BibTeX" file is here: https://github.com/cdk/cdk/blob/master/doc/refs/cheminf.bibx
duplicated MoleculaFormulaRangeManipulatorTest entry
| * | ||
| * @cdk.module test-formula | ||
| */ | ||
| public class MolecularFormulaGeneratorTest extends CDKTestCase { |
There was a problem hiding this comment.
Does this list of tests also include the tests MassToFormulaToolTest?
There was a problem hiding this comment.
I included similar tests to MassToFormulaToolTest (with some modifications), and I also added a few more.
There was a problem hiding this comment.
I would not object against porting the original tests... (wishlist level... not too important)
There was a problem hiding this comment.
I included those MassToFormulaToolTest tests that were relevant to MolecularFormulaGenerator. Or, maybe I don't comprehend your request?
|
Generally looks like a nice and clean patch. I haven't tried compiling it, nor ran the tests, though. |
|
I made a few corrections according to Egon's comments. |
|
Should I merge the commits into a single one and create a new pull request? |
|
No, rather keep them separate as is now: https://github.com/cdk/cdk/pull/103/commits That way, we can just review the updates, rather than everything again. |
|
OK, with the updates, if it compiles, I'm happy for it to go in! Tomas, John, what do you think about the old code? If this new code that all the things the old code does (does it?), maybe the old class should be removed? Tomas, or is the old code an intrinsically different algorithm that may have different applications and we need to ask Miguel if the old code can be removed? |
|
The results (formula sets) generated by both algorithms are (must be) equal. |
|
If existing usages can be replaced (elsewhere in the library) the old one should be moved to the deprecated module rather than removed completely. |
|
Is there a way to have Jenkins report on the usage of deprecated code (outside the testing code)? |
|
The old MassToFormulaTool is not used anywhere in the CDK library. |
|
Okay applied and pushed, just going to move the old version. Two more pointers.
|
|
Why not move the MformulaTests and the old cheminf.bibx to the deprecated(legacy?) module as well? Cleaning up junk makes it easier for newcomers to make contributions. |
Creating a pull request as discussed on the cdk-devel list.