search

opencobra / cobrapy

COBRApy is a package for constraint-based modeling of metabolic networks.
http://opencobra.github.io/cobrapy/
GNU General Public License v2.0
467 stars 218 forks source link

trouble referencing the name variable #1302

Closed tillea closed 7 months ago

tillea commented 2 years ago

Problem description

I've tried to update the Debian packaged version of cobrapy to the latest upstream version 0.26.0. Unfortunately this resulted in build time test errors which you can review in the Debian CI. Nilesh Patra has found a promising patch where he simply replaced an instance of group.name by group.getName() which makes the test pass. I would like you to confirm that this patch is OK.

Kind regards, Andreas.

Midnighter commented 2 years ago

Curious that we never caught that before. Thanks for the patch. I think it should either be group.getName() or cobra_group.name @akaviaLab can you confirm your intentions there?

akaviaLab commented 2 years ago

Either would work, since cobra_group.name is assigned group.getName() higher in the code. I'm partial to group.getName, but pick whichever makes sense to you. Can the patch writer maybe add a test? I've started a new job and I'm swamped, so I can't do that in a reasonable time frame.

On Thu, Nov 24, 2022 at 10:15 AM Moritz E. Beber @.***> wrote:

Curious that we never caught that before. Thanks for the patch. I think it should either be group.getName() or cobra_group.name @akaviaLab https://github.com/akaviaLab can you confirm your intentions there?

— Reply to this email directly, view it on GitHub https://github.com/opencobra/cobrapy/issues/1302#issuecomment-1326236277, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACQYYZXEGNNJQVV7X26ZZSTWJ453HANCNFSM6AAAAAASJ34AMY . You are receiving this because you were mentioned.Message ID: @.***>

tillea commented 2 years ago

Hi,

Am Thu, Nov 24, 2022 at 05:59:16AM -0800 schrieb akaviaLab:

Either would work, since cobra_group.name is assigned group.getName() higher in the code. I'm partial to group.getName, but pick whichever makes sense to you.

Thanks for confirming, I think I'll leave the patch as is inside Debian.

Can the patch writer maybe add a test? I've started a new job and I'm swamped, so I can't do that in a reasonable time frame.

I admit I'm way to uneducated to cobrapy code. All I can say that the issue was spotted when running the cobrapy test suite - I'm wondering in turn why this has not yet caused the issue we observed in the Debian build.

Kind regards, Andreas.

cdiener commented 2 years ago

The proposed change make sense so we can pull it in upstream. Maybe the issue is the older version of libsbml on the Debian CI? We usually only test against the latest libsbml that is compatible.

Midnighter commented 2 years ago

I suspect that we don't actually have a model with groups that we test on? I don't think the libSBML API has changed between versions.

cdiener commented 2 years ago

But the tests in Debian CI seem to fail in the regular test suite. Why would they test different models there? Maybe I'm missing something though...