`parent` not injected into `AMI`

[petermr]

The test org.contentmine.cproject.files.CProjectTest.testPicocliInjectparentOK() runs AMI over a CProject with the command pdfbox and completes. (It's largely a no-op as the data are already maked)

    @Test
    public void testPicocliInjectparentOK() {
        CProject project = new CProject(new File(NAConstants.TEST_AMI_DIR, "battery10"));
        List<String> treeNames = Arrays.asList(new String[] {
                "PMC3776197",
                "PMC4062906",
                });

        String treeNamesString = String.join(" ", treeNames);
        String cmd = "-p " + project
                + " -v"
                + " --includetree " + treeNamesString
                + " pdfbox";
        AMI.execute(cmd);

    }

The same setup with a different command

    @Test
    public void testPicocliInjectParentNPEFail() {
        CProject project = new CProject(new File(NAConstants.TEST_AMI_DIR, "battery10"));
        List<String> treeNames = Arrays.asList(new String[] {
                "PMC3776197",
                "PMC4062906",
                });

        String treeNamesString = String.join(" ", treeNames);
        String cmd = "-p " + project
                + " -v"
                + " --includetree " + String.join(" ", treeNamesString)
                + " image";
        AMI.execute(cmd);

    }

fails with NPE because parent is not set in abstractAMITool.

    Generic values (AMIImageTool)
================================
input basename      null
input basename list null
cproject            /Users/pm286/workspace/cmdev/ami3/src/test/resources/org/contentmine/ami/battery10
ctree               
cTreeList           [src/test/resources/org/contentmine/ami/battery10/PMC3776197, src/test/resources/org/contentmine/ami/battery10/PMC4062906]
excludeBase         null
excludeTrees        null
forceMake           false
includeBase         null
includeTrees        2 [PMC3776197, PMC4062906]
log4j               
verbose             1

Specific values (AMIImageTool)
================================
minHeight           100
minWidth            100
smalldir            small
monochromeDir       monochrome
duplicateDir        duplicate
borders             null
binarize            null
despeckle           false
erodeDilate         false
maxheight           1000
maxwidth            1000
posterize           false
priority            RAW
rotate              null
scalefactor         null
sharpen             none
template            null
threshold           null

Generic values (AMIFilterTool)
================================
java.lang.NullPointerException
    at org.contentmine.ami.tools.AbstractAMITool.getCProjectDirectory(AbstractAMITool.java:427)
    at org.contentmine.ami.tools.AbstractAMITool.validateCProject(AbstractAMITool.java:247)
    at org.contentmine.ami.tools.AbstractAMITool.parseGenerics(AbstractAMITool.java:219)
    at org.contentmine.ami.tools.AbstractAMITool.runCommands(AbstractAMITool.java:196)
    at org.contentmine.ami.tools.AMIImageTool.runPrevious(AMIImageTool.java:373)
    at org.contentmine.ami.tools.AbstractAMITool.runCommands(AbstractAMITool.java:204)
    at org.contentmine.ami.tools.AbstractAMITool.call(AbstractAMITool.java:186)
    at org.contentmine.ami.tools.AbstractAMITool.call(AbstractAMITool.java:1)
    at picocli.CommandLine.executeUserObject(CommandLine.java:1783)
    at picocli.CommandLine.access$900(CommandLine.java:145)
    at picocli.CommandLine$RunLast.executeUserObjectOfLastSubcommandWithSameParent(CommandLine.java:2150)
    at picocli.CommandLine$RunLast.handle(CommandLine.java:2144)
    at picocli.CommandLine$RunLast.handle(CommandLine.java:2108)
    at picocli.CommandLine$AbstractParseResultHandler.execute(CommandLine.java:1975)
    at picocli.CommandLine.execute(CommandLine.java:1904)
    at org.contentmine.ami.tools.AMI.execute(AMI.java:147)
    at org.contentmine.ami.tools.AMI.execute(AMI.java:144)
    at org.contentmine.cproject.files.CProjectTest.testPicocliInjectParentNPEFail(CProjectTest.java:675)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:498)
    at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
    at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
    at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
    at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
    at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
    at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
    at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:63)
    at org.junit.runners.ParentRunner$4.run(ParentRunner.java:331)
    at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79)
    at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329)
    at org.junit.runners.ParentRunner.access$100(ParentRunner.java:66)
    at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293)
    at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
    at org.junit.runners.ParentRunner.run(ParentRunner.java:413)
    at org.eclipse.jdt.internal.junit4.runner.JUnit4TestReference.run(JUnit4TestReference.java:89)
    at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:41)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:541)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:763)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:463)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:209)

Note that it fails in parseGenerics() which is before any main business logic.

(It also causes the NPE reported in the previous distinct issue)

[petermr]

Found the problem by binary chop on Options

    @Option(names = {"--filter"},
            arity = "0",
            defaultValue = "false",
            description = "pre-runs default FILTER (i.e. without args), duplicate, small, monochrome"
            )
    private boolean filter;

It seems that arity = 0 and defaultValue cannot both be present. Probably a stupid thing to write, but hard to track down.

Please check this is a correct diagnosis.

[remkop]

That code seems okay. Boolean options have arity = "0" by default, no need to specify it; default value for primitive boolean options is false by default, no need to specify it; but on the other hand this shouldn’t cause any issues either.

[petermr]

I've committed the code. I'll keep playing. I don't think there's anything special in AMIImageTool I tried to debug by setting a watch on parent but the event happens (or fails to happen) very early.

But I think I have code to continue working with.

[petermr]

Sorry...I was unclear... in the last message (which I have deleted)

deleting arity = "0"

means it works

[remkop]

~~Instead of calling AMI.execute(String[]), please call AMI.execute(Class, String[]) in the tests. The class is the class for the tool under test.~~ Update: Ignore this.

[remkop]

If you still have this issue, please execute with -Dpicocli.trace=DEBUG and post the output.

[remkop]

I debugged AMIImageTool and found the cause:

protected void runPrevious() {
    if (filter) {
        new AMIFilterTool().setCProject(cProject).setCTree(cTree).runCommands();
    }
}

This is instantiating a new AMIFilterTool instance and calling runCommands on it. Previously, calling setCProject(cProject).setCTree(cTree) was sufficient to initialize that FilterTool object, but now it is not any more: AMIFilterTool also extends AbstractAMITool, so if any logic in AMIFilterTool calls getCProjectDirectory then we get a NPE since the parent field is null in the AMIFilterTool instance.

[remkop]

Continuing on my previous comment: to support code like this

new AMIFilterTool().setCProject(cProject).setCTree(cTree).runCommands();

We need to change AbstractAMITool::validateCProject: if it is possible that the CProject was set externally then we don't need to create one. Currently validateCProject will always try to create a new CProject if getCProjectDirectory returns a non-null value.

One change: don't create a CProject if one already exists.

    protected void validateCProject() {
        // next 3 lines are new
        if (cProject != null) { // if we already have a CProject we don't need to create one
            return;
        }
        // below is existing code
        if (getCProjectDirectory() != null) {
            if (makeCProjectDirectory) {
                new File(getCProjectDirectory()).mkdirs();
            }
            File cProjectDir = new File(getCProjectDirectory());
            setCProjectDirectory(checkDirExistenceAndGetAbsoluteName(cProjectDir, "cProject"));

            if (getCProjectDirectory() != null) {
                cProject = new CProject(cProjectDir);
                cTreeList = generateCTreeList();
            } else {
                System.err.println(""
                        + "************************\n"
                        + "WARNING: CProject directory does not exist\n"
                        + "************************\n");
            }
        }
    }

Another thing is: why have separate fields for CProject and CProject directory? Why not have a single CProject field and ask that CProject for its directory if necessary? That way they can never go out of sync.

We can accomplish this with a custom type converter that creates a CProject instance from the value specified for the --cproject option (the path). Similarly for CTree.

[petermr]

Thanks so much. Please feel free to go ahead and change whatever is needed. I wiil work by example and won't try my own ideas.

The problems arise because I am opening up a few subcommands for images. Some of the code is pre picocli and i try to tidy as I bring them in.

I have loaded these classes and I think image is the only one that causes problems. Ami image is a complex system and might later be split with subcommands but I'm not doing that just now.

I am trying to remove dead tests and use of external files. But this will take months.

This is so valuable . Thanks . More later

On Sat, 9 May 2020, 06:20 Remko Popma, notifications@github.com wrote:

Continuing on my previous comment: to support code like this

new AMIFilterTool().setCProject(cProject).setCTree(cTree).runCommands();

We need to change AbstractAMITool::validateCProject: if it is possible that the CProject was set externally then we don't need to create one. Currently validateCProject will always try to create a new CProject if getCProjectDirectory returns a non-null value.

One change: don't create a CProject if one already exists.

protected void validateCProject() { // next 3 lines are new if (cProject != null) { // if we already have a CProject we don't need to create one return; } // below is existing code if (getCProjectDirectory() != null) { if (makeCProjectDirectory) { new File(getCProjectDirectory()).mkdirs(); } File cProjectDir = new File(getCProjectDirectory()); setCProjectDirectory(checkDirExistenceAndGetAbsoluteName(cProjectDir, "cProject"));

      if (getCProjectDirectory() != null) {
          cProject = new CProject(cProjectDir);
          cTreeList = generateCTreeList();
      } else {
          System.err.println(""
                  + "************************\n"
                  + "WARNING: CProject directory does not exist\n"
                  + "************************\n");
      }
  }

}

Another thing is: why have separate fields for CProject and CProject directory? Why not have a single CProject field and ask that CProject for its directory if necessary? That way they can never go out of sync.

We can accomplish this with a custom type converter https://picocli.info/#_custom_type_converters that creates a CProject instance from the value specified for the --cproject option (the path). Similarly for CTree.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/petermr/ami3/issues/41#issuecomment-626108383, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFTCS7Y6WOVQ5VLNY6QYCDRQTR3DANCNFSM4M4HIDQQ .

[remkop]

Okay. I am about to do new release for picocli 4.3. After that I expect to have more time again to help with ami.

[petermr]

Great! I hope that these experiences are helping exercise picocli. Feel free to introduce any new features / versions / java versions You see how much the EU values open source.

I'm concentrating on checking the AMIImage functionality (image/pixel/ocr) and also copntinuing to download and transform documents.

On Sat, May 9, 2020 at 8:35 AM Remko Popma notifications@github.com wrote:

Okay. I am about to do new release for picocli 4.3. After that I expect to have more time again to help with ami.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/petermr/ami3/issues/41#issuecomment-626122564, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFTCS3PWBY2EQY2KERO5HDRQUBV5ANCNFSM4M4HIDQQ .

-- Peter Murray-Rust Founder ContentMine.org and Reader Emeritus in Molecular Informatics Dept. Of Chemistry, University of Cambridge, CB2 1EW, UK

[remkop]

I committed a change that makes the NPE go away in the test for AMIImageTool. This does not address the root cause; subclasses of AbstractAMITool are not meant to be instantiated and used directly .

[petermr]

On Sat, May 9, 2020 at 11:40 AM Remko Popma notifications@github.com wrote:

I committed a change that makes the NPE go away in the test for AMIImageTool. This does not address the root cause; subclasses of AbstractAMITool are not meant to be instantiated and used directly .

Understood. I think tests seem to stress the encapsulation. Can this be done with protected constructors, or at least run-time warnings?

One probable cause is the duplication of AMIImageFilter and AMIFilter. I have removed the first.

You can see the logic in the ForestPlot test (on this channel). The flow is: pdfbox - which generates raw images filter which removes unwanted images image which actually processes them

In the past we had image-filter and image-transform Options and methods

now we have the sequence.

Any suggestions welcome.

This also overlaps with runPrevious which is a primitive make system not completely developed.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/petermr/ami3/issues/41#issuecomment-626146338, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFTCS4FR332IDWEWMLAHU3RQUXI3ANCNFSM4M4HIDQQ .

-- Peter Murray-Rust Founder ContentMine.org and Reader Emeritus in Molecular Informatics Dept. Of Chemistry, University of Cambridge, CB2 1EW, UK