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

SINGE shell script and separation of GLG_Instance and Aggregate functions #37

Merged
merged 52 commits into from
Sep 22, 2019

Conversation

atuldeshpande
Copy link
Collaborator

@atuldeshpande atuldeshpande commented Aug 15, 2019

Closes #17

This pull request creates a shell script to emulate the standalone SINGE function which calls the GLG_Instance executable followed by Aggregate executable.
Requires compilation of the code/GLG_Instance.m and code/Aggregate.m. "v94" in the standalone_SCINGE.sh should be replaced by the name of the folder containing the runtime libraries.

Copy link
Member

@agitter agitter left a comment

Choose a reason for hiding this comment

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

What changed in data1/tf.mat? Can we avoid changing the input data in this pull request?

Do we want to still keep SCINGE_Example.m or is that stale now? The main reason to keep it would be to give an example for users who want to run SINGE locally and directly through MATLAB instead of using the compiled code, right?

Can we rename SCINGE.m and any other source files that use the old name in this pull request?

Do we want to change test cases to run standalone_SCINGE.sh on the sample data? That's the primary workflow now. We could also test that and the compiled SCINGE_Example.

code/Aggregate.m Outdated Show resolved Hide resolved
code/SCINGE.m Outdated Show resolved Hide resolved
code/Aggregate.m Outdated Show resolved Hide resolved
standalone_SCINGE.sh Outdated Show resolved Hide resolved
standalone_SCINGE.sh Outdated Show resolved Hide resolved
standalone_SCINGE.sh Outdated Show resolved Hide resolved
@agitter agitter mentioned this pull request Aug 17, 2019
@atuldeshpande
Copy link
Collaborator Author

We can run the equivalent of SINGE_Example.m by running the following command:
./standalone_SCINGE.sh data1/X_SCODE_data.mat data1/tf.mat testOutput tests/example_hyperparameters.txt

Perhaps we can include the above in the README.md?

@agitter
Copy link
Member

agitter commented Aug 20, 2019

Perhaps we can include the above in the README.md?

Yes, let's add that command to the readme when we document how to use this new script and updated our input parameter descriptions.

@agitter
Copy link
Member

agitter commented Aug 21, 2019

I finished a round of changes to the compile script, Dockerfile, and Docker-based tests. I'll see what errors appear when these tests run.

I restored SINGE_Example.m. I'd like to keep this as example usage for anyone who wants to use SINGE directly through MATLAB. We'll have to document both options (MATLAB and script) in the readme.

@agitter
Copy link
Member

agitter commented Aug 21, 2019

I had to specify the MATLAB runtime path in the standalone script so that it could find the runtime in the Docker container. Should we make that another argument to the script? The location will vary for different users.

standalone_SINGE.sh Outdated Show resolved Hide resolved
@atuldeshpande
Copy link
Collaborator Author

If someone provides the argument -lambda, will it be truncated as ambda?

Yes. That's how it will handle currently.

@agitter
Copy link
Member

agitter commented Sep 7, 2019

The new parameter parser broke the support for one of the date formats:

Error using parseParams (line 47)
The value of 'date' is invalid. It must satisfy the function: @(x)any(ismember({datestr(datenum(x),['mm/dd/yyyy']),datestr(datenum(x))},x)).
Error in SINGE_GLG_Test (line 9)

See https://travis-ci.com/gitter-lab/SINGE/builds/126320790#L698

@agitter
Copy link
Member

agitter commented Sep 14, 2019

@atuldeshpande it looks like the error here https://travis-ci.com/gitter-lab/SINGE/jobs/235318717#L989 is being caused by SINGE.m. The call to SINGE_GLG_Test doesn't pass the output directory so it defaults to Output. That causes SINGE_Aggregate to crash if a non-default output directory is used.

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@agitter agitter merged commit ccdc523 into master Sep 22, 2019
@agitter agitter deleted the config_changes branch September 22, 2019 12:08
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.

Command line interface for SINGE
2 participants