search

kdahlquist / GRNmap

Gene Regulatory Network modeling and parameter estimation
BSD 3-Clause "New" or "Revised" License
4 stars 3 forks source link

threshold_b should be 0 if there are no inputs controlling that gene #289

Closed kdahlquist closed 6 years ago

kdahlquist commented 7 years ago

@Nwilli31 found that in the output for the dCIN5 network, there is a value for threhold_b for a gene that does not have any controllers. For a gene that has no controllers, b should not be estimated and should have a value of 0. It looks like the production rate is being copied into that cell in the output file and then there is a zero to the immediate right to it. Hopefully this is just a problem with writing the output and not with the computation.

Sample files demonstrating the bug will be forthcoming.

kdahlquist commented 6 years ago

I'm attaching an output file that shows the results of the bug. This file came from the L-curve run this summer, so it was replicated with the current master code.

When verifying the bug and writing the test, you should make a much simpler input network/data and not directly use the data workbook for this.

15-genes_28-edges_db5_Sigmoid_estimation_missing-values_L-curve_1_output.xlsx

dondi commented 6 years ago

Initial inspection reveals https://github.com/kdahlquist/GRNmap/blob/beta/matlab/output.m#L122 where a variable outputpro appears to be reused when outputting threshold_b.

The ideal scenario here, in our on-going quest to decrease technical debt, we should:

  1. Write a failing test that catches this extra column
  2. Doing this may require refactoring---report back after exploring this
  3. Actually fix the bug and watch the test go green
dondi commented 6 years ago

@cazinge reported that it does not appear to be practical at the moment to implement the test in a way that does not involve input/output files. He is thus clear to write a test that does involves these files; try to minimize the content of the files to solely the data needed to reproduce the bug.

kdahlquist commented 6 years ago

I have verified that this bug is fixed in the beta code from 11/13/17. I don't want to close the issue until the unit tests are written.

im-deepfriedwater commented 6 years ago

See #386

kdahlquist commented 6 years ago

@jtorre39 is still working on this one because discovered that L-curve test was failing and had to go back and figure out at which point it started failing.

kdahlquist commented 6 years ago

Key to look for when reproducing this bug: For a gene that has no controllers, b should not be estimated and should have a value of 0. For example, PHD1 in demo files #3/4 on GRNsight has no inputs, only outputs. It has all zeros in the row for PHD 1 in the adjacency matrix. The bug was that it seemed like the production rate was being copied into the optimized_threshold_b sheet when the value should have been zero.

This was reported a long time ago, so step1 should be reproducing the bug. @jtorre39 said that @cazinge only touched the L-curve code, so he will need to look in output.

im-deepfriedwater commented 6 years ago

Took one more look and found changes Eddie had made to output.m! I reverted the changes and made a test which failed, and then put the changes back and the tests are now working. The changes @cazinge made to fix this issue now have a test for it and they are verified to work!

kdahlquist commented 6 years ago

Awesome!

Kam Dahlquist Professor of BiologyLoyola Marymount University -------- Original message --------From: Justin Kyle Torres notifications@github.com Date: 2/23/18 6:44 PM (GMT-08:00) To: kdahlquist/GRNmap GRNmap@noreply.github.com Cc: Kam Dahlquist kdahlquist@lmu.edu, Author author@noreply.github.com Subject: Re: [kdahlquist/GRNmap] threshold_b should be 0 if there are no   inputs controlling that gene (#289) Took one more look and found changes Eddie had made to output.m! I reverted the changes and made a test which failed, and then put the changes back and the tests are now working. The changes @cazinge made to fix this issue now has a test for it and it is verified to work!

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/kdahlquist/GRNmap","title":"kdahlquist/GRNmap","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/kdahlquist/GRNmap"}},"updates":{"snippets":[{"icon":"PERSON","message":"@jtorre39 in #289: Took one more look and found changes Eddie had made to output.m! I reverted the changes and made a test which failed, and then put the changes back and the tests are now working. The changes @cazinge made to fix this issue now has a test for it and it is verified to work!"}],"action":{"name":"View Issue","url":"https://github.com/kdahlquist/GRNmap/issues/289#issuecomment-368192627"}}}

bengfitzpatrick commented 6 years ago

notes in "Addressing Issues" comment above. tests now passing for b=0. closing.