thpar / diffany

Diffany is an open-source toolbox for calculating and visualizing differential networks.
7 stars 2 forks source link

Reference required when it wasn't selected #233

Closed diffany-admin closed 9 years ago

diffany-admin commented 9 years ago

Issue by svlandeg Mon Nov 24 12:17:04 2014


I got this bug:

refrequired

@tvparys: apparently, the refRequired parameter was somewhere put to true, but I'm not sure whether this is wrong on my end or yours?

diffany-admin commented 9 years ago

Comment by tvparys Mon Nov 24 12:19:29 2014


Ref required should always be read from the Diffany Tab. On your screenshot it seems to be false, so it should be set as false when sent to the algorithm too.

diffany-admin commented 9 years ago

Comment by svlandeg Mon Nov 24 12:20:38 2014


That's what I thought. Still, somewhere downstream a "true" is read.

diffany-admin commented 9 years ago

Comment by tvparys Mon Nov 24 12:28:11 2014


Hmmmm... This might not be updated at some point after all.

diffany-admin commented 9 years ago

Comment by svlandeg Mon Nov 24 12:30:54 2014


The error is thrown by EdgeByEdge.calculateConsensusNetwork

if (refRequired && refNetworks.size() != 1) { String errormsg = "Please define exactly 1 reference network (" + refNetworks.size() + " found) or change the refRequired parameter to false!"; throw new IllegalArgumentException(errormsg); }

This method is only called from CalculateDiff.calculateConsensusNetwork which in turn is called by CalculateDiff.calculateOneDifferentialNetwork

In that last method, the parameter is read from the RunConfiguration: RunConfiguration rc = p.getRunConfiguration(runID); rc.getRefRequired()

diffany-admin commented 9 years ago

Comment by tvparys Mon Nov 24 12:31:47 2014


Yeah, but it's a bug in the GUI code:

if (model.getSelectedProject()!=null 
                && !model.isGenerateDiffNets() 
                && model.getSelectedProject().getReferenceNetwork()!=null
                && newNumberOfTicks>2){
            this.requireRefNetCheckBox.setEnabled(true);
            this.requireRefNetCheckBox.setSelected(true);
        } else {
            this.requireRefNetCheckBox.setEnabled(false);
            this.requireRefNetCheckBox.setSelected(false);
        }
diffany-admin commented 9 years ago

Comment by tvparys Mon Nov 24 12:32:12 2014


I change the status of the checkbox here, without updating the model.

diffany-admin commented 9 years ago

Comment by svlandeg Mon Nov 24 12:32:55 2014


I figured it must have been something like that ... happy you found it!

diffany-admin commented 9 years ago

Comment by tvparys Mon Nov 24 12:34:04 2014


We really need more emoticons in GitHub. Right now, I'd like a squashed bug or something.

diffany-admin commented 9 years ago

Comment by svlandeg Mon Nov 24 12:37:50 2014


:bug: :hammer:

diffany-admin commented 9 years ago

Comment by tvparys Mon Nov 24 12:43:41 2014


Woah! I never knew! I thought those 4 examples when hitting ":" were it! This actually boosted my productivity and overall happiness. :ant:

diffany-admin commented 9 years ago

Comment by svlandeg Mon Nov 24 12:44:27 2014


I'm guessing you are fixing this all at once, but a similar thing happens e.g. when I have the box unselected, and then select "Differential networks". This should automatically switch the refReq box on too, but that box is grey and remains unselected, though the model probably has put it to true

diffany-admin commented 9 years ago

Comment by tvparys Mon Nov 24 12:47:15 2014


Yes... I tweak the GUI according to certain circumstances, but failed to consistently update the model. I was assuming that upon run, the model read the status of the GUI, which is not true. The algorithm reads the status of the (unaltered) model.

diffany-admin commented 9 years ago

Comment by svlandeg Mon Nov 24 12:48:53 2014


:see_no_evil:

diffany-admin commented 9 years ago

Comment by tvparys Mon Nov 24 13:03:10 2014


svlandeg:

when I have the box unselected, and then select "Differential networks". This should automatically switch the refReq box on too, but that box is grey and remains unselected, though the model probably has put it to true

Can you reproduce that? For me the refReq is always gray and ON when Differential Networks are requested.

diffany-admin commented 9 years ago

Comment by tvparys Mon Nov 24 13:05:57 2014


It would be very useful if the Logger started with a full list of used parameters for the run (generate Diff and/or Consensus, require refNet for consensus, one-to-all or pairwise, consensus edge cutoff, ...)

diffany-admin commented 9 years ago

Comment by svlandeg Mon Nov 24 13:11:22 2014


Agreed, I'll open an issue

diffany-admin commented 9 years ago

Comment by svlandeg Mon Nov 24 13:16:54 2014


reproducing, good question. I can't recall exactly, but this may only occur in the limbo state of having no reference network, but "differential networks" switched on. In that case, I can't run, and the refReq box is grey and off. Considering this is not a valid configuration, it might not matter much. When I select a reference network, the refReq box is correctly switched on (and kept grey)

diffany-admin commented 9 years ago

Comment by tvparys Mon Nov 24 13:18:02 2014


Going over the code now, but it seems like the only actual GUI/model inconsistency was that refReq checkbox.

diffany-admin commented 9 years ago

Comment by svlandeg Fri Nov 28 08:28:10 2014


I wonder whether this bug is back :(

refreq

diffany-admin commented 9 years ago

Comment by svlandeg Fri Nov 28 08:30:24 2014


Only this time, it looks like the reference network wasn't found, even though the box was ticked.

The way I got this bug: I included both my input networks (first 2) and selected the reference. I kept all default parameters and hit "Start", thus producing the differential network (only), without a problem. I went back, deselected "Differential networks" and selected "Consensus networks", in the meantime not changing any of the Reference (required) tickboxes. I hit run, and got this error.

diffany-admin commented 9 years ago

Comment by svlandeg Fri Nov 28 08:39:24 2014


This seems to happen every time I select "1-all" and consensus networks, while at the same time deselecting "differential networks".

diffany-admin commented 9 years ago

Comment by svlandeg Fri Nov 28 09:00:38 2014


@tvparys: I can't find a dependency in my code between the differential and consensus calculation which could explain this behaviour, and tests mimicing this behaviour from the command line are not producing the error either - can you look into this?

diffany-admin commented 9 years ago

Comment by tvparys Fri Nov 28 10:30:27 2014


Bug confirmed.

diffany-admin commented 9 years ago

Comment by svlandeg Fri Nov 28 10:35:38 2014


Is there any possibility that you cast the networks to InputNetworks and not keep the original ReferenceNetwork as actual type of the class? Because that's how my code finds the reference network...

diffany-admin commented 9 years ago

Comment by tvparys Fri Nov 28 10:43:10 2014


No, that looks good to me. Every change in the JTable is immediately reflected in the Project:

ReferenceNetwork net = CyNetworkBridge.getReferenceNetwork(network, this.project.getEdgeOntology());
this.project.registerSourceNetwork(net, new Logger());
diffany-admin commented 9 years ago

Comment by tvparys Fri Nov 28 12:13:55 2014


input networks in runtime config when calculating only consensus networks:

Input class: class be.svlandeg.diffany.core.networks.InputNetwork
Input class: class be.svlandeg.diffany.core.networks.InputNetwork

input networks in runtime config when calculating only differential networks:

Input class: class be.svlandeg.diffany.core.networks.ConditionNetwork
Input class: class be.svlandeg.diffany.core.networks.ReferenceNetwork
diffany-admin commented 9 years ago

Comment by tvparys Fri Nov 28 12:33:34 2014


@svlandeg : Help me out here. When generating the RunConfiguration in the CyProject, everything is still fine. We have a Reference and a Condition network. These are passed to project.addRunConfiguration() in a Set<InputNetwork>

The moment I retrieve the runConfig in order to execute the task, the InputNetworks have lost their subclasses.

diffany-admin commented 9 years ago

Comment by svlandeg Fri Nov 28 12:35:39 2014


Could it be that you're constructing a RunConfiguration, but you would be constructing a RunDiffConfiguration if the "generate diff networks" checkbox was selected?

diffany-admin commented 9 years ago

Comment by svlandeg Fri Nov 28 12:38:56 2014


That is exactly it. Even though I always tried to make sure a ReferenceNetwork would be kept as such and treated as an InputNetwork, the Cleaning step goes off and creates an InputNetwork clone. Blast!

diffany-admin commented 9 years ago

Comment by tvparys Fri Nov 28 12:39:22 2014


Since when are there different runconfigurations?

When diffnets should be generated:

runConfigID = project.addRunConfiguration(refNet, condNets, model.getOverlapSupportCutoff(), true, listener);

when only consensus net should be generated:

runConfigID = project.addRunConfiguration(inputNetworks, model.getOverlapSupportCutoff(), model.isRefIncludedInOverlapSupportCutoff(), listener);

I use the method there that doesn't require the distinction between ref/cond as there is not necessarily a ref needed.

diffany-admin commented 9 years ago

Comment by tvparys Fri Nov 28 13:40:22 2014


Ah, aha, so you've found it? :+1:

diffany-admin commented 9 years ago

Comment by svlandeg Fri Nov 28 13:40:58 2014


I am pretty sure. Cf. #237, closing this one again.

diffany-admin commented 9 years ago

Comment by svlandeg Fri Nov 28 13:41:00 2014


"Since when are there different runconfigurations?" --> you shouldn't know or care about that, sorry ;-)