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

optEnvelope #2330

Merged
merged 28 commits into from
Nov 7, 2024
Merged

optEnvelope #2330

merged 28 commits into from
Nov 7, 2024

Conversation

Kretaks
Copy link
Contributor

@Kretaks Kretaks commented Oct 2, 2024

Please include a short description of enhancement here

Added a design tool called optEnvelope: https://doi.org/10.1371/journal.pone.0294313
Added a test for optEnvelope

I hereby confirm that I have:

  • Tested my code on my own machine
  • Followed the guidelines in the Contributing Guide
  • Selected develop as a target branch (top left drop-down menu)

(Note: You may replace [ ] with [X] to check the box)

Copy link
Member

@rmtfleming rmtfleming left a comment

Choose a reason for hiding this comment

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

Numerous functions without headers.
milpOEReinserts

Try this to start with
which printFunctionIO

@Kretaks Kretaks requested a review from rmtfleming October 3, 2024 07:36
@Kretaks
Copy link
Contributor Author

Kretaks commented Oct 3, 2024

@rmtfleming
Added headers to functions

@rmtfleming
Copy link
Member

Follow the style of printFunctionIO
https://opencobra.github.io/cobratoolbox/latest/contributing.html#code
headers should show what type of input is expected, e.g., cell arrray, numeric, etc. with dimensions

@Kretaks
Copy link
Contributor Author

Kretaks commented Oct 8, 2024

@rmtfleming
Changes have been made.

Copy link
Collaborator

@farid-zare farid-zare left a comment

Choose a reason for hiding this comment

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

Could you please address these reviews?

tol = 1e-6;

% define the solver packages to be used to run this test
solversPkgs = {'gurobi'};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please change this section to define 'gurobi' as a required solver first and then use prepareTest function as here in the template:
https://github.com/opencobra/cobratoolbox/blob/master/docs/source/guides/testTemplate.m
By using this method the test would be skipped where gurobi is not installed instead of failing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@farid-zare
defined gurobi as required and
added prepareTest function

% This function is going through inactive reactions sequentially and
% reinserting them one by one to get best possible set of knockouts while
% retaining optimal envelope. With numTries parameter this can be done
% multiple times by randomizing list of inactive reactions
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please also include a USAGE section in your documentation as:
% the end of the description
%
% USAGE:
%
% [output1, output2, output3] = someFunction(input1, input2, input3)
%
% here the other section can begin

examples of this is available in other cobra toolbox codes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@farid-zare
I am using "EXAMPLE:" in other functions so I changed this one to include "EXAMPLE:" just before "AUTHORS:"
Let me know if I need to change all "EXAMPLE:" to "USAGE:" for all functions

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for your reply. Function documents must contain USAGE section but EXAMPLE is optional. Including both is recommended. Here is an example of how a standard documentation should look alike, please follow such structure for all of your functions:
https://github.com/opencobra/cobratoolbox/blob/master/src/analysis/FVA/fastFVA/fastFVA.m

@Kretaks
Copy link
Contributor Author

Kretaks commented Oct 28, 2024

@farid-zare
Added "USAGE" for functions

@farid-zare
Copy link
Collaborator

Ok, thanks, I just updated two of the USAGE sections. The EXAMPLE section is a specific example whereas the USAGE section must illustrate a general function call format.
Looks good now.

@rmtfleming rmtfleming merged commit fa2adac into opencobra:develop Nov 7, 2024
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants