Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added warning to inform users of known bug and issues fastcc.py #1427

Open
wants to merge 5 commits into
base: devel
Choose a base branch
from

Conversation

dagl1
Copy link

@dagl1 dagl1 commented Feb 19, 2025

Copy link
Member

@Midnighter Midnighter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution 🙂. I suggest a slightly different wording of the warning.

@@ -2,6 +2,8 @@

from typing import TYPE_CHECKING, List, Optional

from warnings import warn
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linting will complain about the empty line above.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I think I might not understand, the original is:

""Provide an implementation of FASTCC."""

from typing import TYPE_CHECKING, List, Optional

from optlang.symbolics import Zero

from .helpers import normalize_cutoff


Should there not be an empty line between the built-in functions, but empty lines between: built-ins, other packages, other modules? If so I understand, otherwise I am a bit lost

Copy link
Member

@cdiener cdiener left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The warning should probably also point towards find_blocked_reactions which is slower but works.

@dagl1
Copy link
Author

dagl1 commented Feb 21, 2025

The warning should probably also point towards find_blocked_reactions which is slower but works.

Done, this is FVA based right, however doesn't use loopless FVA if I am not mistaken (I am aware that some definitions of blocked reactions don't concern themselves with loops, but in reality any isolated offshoot that forms thermodynamically infeasible loop, is still blocked, but can carry flux at the same time).?

Regarding the error, could you point me in the right direction, it is a bit unclear to me why that 1 ubuntu gives errors while the rest of the workflow was fine?

@cdiener
Copy link
Member

cdiener commented Feb 21, 2025

Yes that does not use loopless FVA. FastCC should also allow loops I think, because all those parsimony-based methods will only avoid loops when the objective itself is not part of a loop.

You could use add_loopless first and then run find_blocked_reactions but it would be very slow.

The CI is complaining because of black. Running black on the changed file before pushing should fix this.

@dagl1
Copy link
Author

dagl1 commented Feb 21, 2025

Yes that does not use loopless FVA. FastCC should also allow loops I think, because all those parsimony-based methods will only avoid loops when the objective itself is not part of a loop.

You could use add_loopless first and then run find_blocked_reactions but it would be very slow.

The CI is complaining because of black. Running black on the changed file before pushing should fix this.

Okay I don't understand what I am doing wrong (sorry....). I ran black on the script, it reformatted a single line, and it is sucessfully committed. I feel I am wasting a lot of your time with this ;// , but also am not entirely sure where the issue comes from

@cdiener
Copy link
Member

cdiener commented Feb 21, 2025

You are all good. Has nothing to do with you. Black got updated yet again and now formats files differently. The affected files have nothing to do with your PR so just ignore this for now.

@Midnighter We should pin the black version in tox because that is happening in almost every PR now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FastCC results differ from respective MATLAB function
3 participants