Patch/reorg 12 feb17 #268
Merged
Patch/reorg 12 feb17 #268
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…th SMIRKS (via AMBIT atm)
…ve it there and make non-public.
…nd libiocml... so for now we need to put it in io.
…ue it is more efficient to use SMILES (or even InChI with FixedH). To map the atom correspondence to the InChI we still have to fall back to substructure matching but the correct approach of using the AuxInfo can also be used.
… in. We can't remove the old method that forces slow execution but it has now be deprecated. New tests added to demonstrate correct exectution.
…ate, a simple update would not be sufficient due to API differences.
…to make cdk-qsarmolecular depend on the fingerprint module which is reasonable since fingerprints are a molecular descriptor.
egonw
requested changes
Feb 14, 2017
| * | ||
| * You should have received a copy of the GNU Lesser General Public License | ||
| * along with this program; if not, write to the Free Software | ||
| * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. | ||
| * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 U |
There was a problem hiding this comment.
Odd not sure where there came from.
| * | ||
| * You should have received a copy of the GNU Lesser General Public License | ||
| * along with this program; if not, write to the Free Software | ||
| * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 U |
| * | ||
| * You should have received a copy of the GNU Lesser General Public License | ||
| * along with this program; if not, write to the Free Software | ||
| * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. | ||
| * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 U |
| import org.openscience.cdk.silent.Atom; | ||
| import org.openscience.cdk.silent.AtomContainer; | ||
| import org.openscience.cdk.silent.Bond; | ||
| import org.openscience.cdk.silent.SilentChemObjectBuilder; |
There was a problem hiding this comment.
Mmm... it was using that, so not a request to fix it... but I should have another round of making things independent from implementation classes :)
There was a problem hiding this comment.
I actually disagree with that design in general, but regardless in this case it is
a) in a test class (i.e. it is already decoupled)
b) deprecated
There was a problem hiding this comment.
(yes, tests actually are fine... actually, I find it hard to see in which file changes are in the web gui... overlooked that; but this was not a change request in the first place...)
|
Fixed |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
First batch of reorganisations and spring cleaning.
API Changes
There are some API changes here in terms of where classes are located (in which module) which I feel are acceptable as including
cdk-bundlewill still allow compilation.I did move and encapsulate the
MathToolsclass as this was only used by a specific class and should not have been public in the first place. Again I feel this change is acceptable, as a rule of thumb I would say "is it chemistry related? Should or could someone expect to be calling this method". Ideally I would encapsulate the EStateFragments class as well but I feel this could be used externally.Deprecation
I've added some more deprecation warnings to old classes.