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

Pull request for adding simple command line based tutorial. #282

Merged
merged 23 commits into from
Jul 2, 2024

Conversation

BJackal
Copy link
Contributor

@BJackal BJackal commented Jun 5, 2024

Added two new tutorials for executing Chaste tests with command line arguments through bash scripts.

The first is a very simple Hello World style example which takes in multiple command line arguments and outputs the sum. For now this has been placed in global as it seemed the most appropriate location.

Another example has been added for running a bash script with command line arguments to execute multiple vertex based simulations with the height and width of tissue based on command line parameters. This is a slightly expanded version of the vertex based simulation tutorial.

BJackal and others added 9 commits May 23, 2024 17:35
…pt for a test. Add those values then cout the result.

Test needs to be ran from bash script and not directly. Will add some kind of assertion that a command line option has been taken otherwise ERROR out.
…s and accompanying test files. Both tests utilise bash scripts and are found one directory above the respective test.

A generic command line arguement example has been added alongside a use case for a vertex mesh.
Adding some extra comments.
Improving commenting.
Slight editing of comments.
Removing null assertion for now as it breaks due to outp not being a pointer. Will be added back in later.
Changing test locations with recent changes to Chaste
* be used to run multiple tests with varying parameter inputs.
*/

// NOTE: This test will not work if directlly executed from terminal due to requiring command line arguements.
Copy link
Member

@mirams mirams Jun 5, 2024

Choose a reason for hiding this comment

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

Sounds as if it will make the test pack fail as well then? We can't really be having that, might need to put heads together on how to do a tutorial a different way...

It should be easy to do an "overwriting defaults" using e.g.

unsigned outp1 = 0;
if CommandLineArguments::Instance()->OptionExists("-opt1")
{
    outp1 = CommandLineArguments::Instance()->GetUnsignedCorrespondingToOption("-opt1");
}

and then we can put in comments saying "Please rerun the executable with arguments to see how the output changes, but note you will have to do this outside the ctest framework".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can think of one alternative way. The test could have some default conditions that are ran if command line arguments are not detected. Allowing the tutorial to simply run with those default initial conditions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will similarly be changed for TestCommandLineArgumentsTutorial

Copy link
Member

@mirams mirams left a comment

Choose a reason for hiding this comment

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

A few suggestions, I think I would simplify quite a bit as there's nothing vertexey or cell based about wanting to pass in arguments, and would complicate the tutorial to show off more of the capabilities of the CommandLineArguments class!

global/test/TestCommandLineArgumentsTutorial.hpp Outdated Show resolved Hide resolved
global/test/TestCommandLineArgumentsTutorial.hpp Outdated Show resolved Hide resolved
Modified command line global tutorial to have two tests. First for simple command line arguments and second showing off taking in doubles and utilising a vector
of arguments.

Each test now also checks if command line options have been given and if not will default to a basic set of parameters to ensure the test will run correctly.

Included a paste of the bash script within the test for tutorial purposes.
@BJackal
Copy link
Contributor Author

BJackal commented Jun 7, 2024

  1. Removed the vertex based simulation and its accompanying bash script.
  2. Added a secondary test for the TestCommandLineArgumentsTutorial using a vector of doubles as the input.
  3. Added a commented section to each test with the required bash script to run them, by default left the simpler test as the pre-existing bash script.
  4. Changed int in the first test to unsigned as suggested.
  5. The bash script to take in doubles requires awk (or a similar other tool) to allow for doubles to be input as arguments.
  6. Added a default case for each test if the correct corresponding command line argument is not detected to prevent the test failing.

Possible further additions I can think of now, it may be best to add some simple testing into the default cases or exceptions/assertions to ensure it is running as expected. Additionally , as the CommandLineArguments has the GetBoolCorrespondingToOption method should a test just showing this also be added ?

@BJackal BJackal requested a review from mirams June 7, 2024 12:24
Copy link
Member

@mirams mirams left a comment

Choose a reason for hiding this comment

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

A few tweaks @BJackal I think it is getting there, the main one is probably to just get the tutorial to put the bash script in a code block, and give instructions to copy and paste into a bash script in the build folder, then you can remove the script and the need for hardcoded paths I think.

global/test/TestCommandLineArgumentsTutorial.hpp Outdated Show resolved Hide resolved
global/test/TestCommandLineArgumentsTutorial.hpp Outdated Show resolved Hide resolved
global/test/TestCommandLineArgumentsTutorial.hpp Outdated Show resolved Hide resolved
global/test/TestCommandLineArgumentsTutorial.hpp Outdated Show resolved Hide resolved
global/test/TestCommandLineArgumentsTutorial.hpp Outdated Show resolved Hide resolved
global/test/TestCommandLineArgumentsTutorial.hpp Outdated Show resolved Hide resolved
global/test/TestCommandLineArgumentsTutorial.hpp Outdated Show resolved Hide resolved
global/test/TestCommandLineArgumentsTutorial.hpp Outdated Show resolved Hide resolved
global/test/TestCommandLineArgumentsTutorial.hpp Outdated Show resolved Hide resolved
global/test/TestCommandLineArgumentsTutorial.hpp Outdated Show resolved Hide resolved
@mirams
Copy link
Member

mirams commented Jun 7, 2024

(you can manually run the generate tutorials and website build to see how the generated webpage looks following these instructions: #193 (comment))

BJackal and others added 3 commits June 7, 2024 14:43
Committing suggested edits.

Co-authored-by: Gary Mirams <gary.mirams@gmail.com>
Committing suggested edits.

Co-authored-by: Gary Mirams <gary.mirams@gmail.com>
Committing suggested edits.

Co-authored-by: Gary Mirams <gary.mirams@gmail.com>
BJackal and others added 6 commits June 7, 2024 14:44
Committing suggested edits.

Co-authored-by: Gary Mirams <gary.mirams@gmail.com>
Committing suggested edits.

Co-authored-by: Gary Mirams <gary.mirams@gmail.com>
Committing suggested edits.

Co-authored-by: Gary Mirams <gary.mirams@gmail.com>
Committing suggested tidying.

Co-authored-by: Gary Mirams <gary.mirams@gmail.com>
Committing suggested edits.

Co-authored-by: Gary Mirams <gary.mirams@gmail.com>
… ran from inside the build directory.

Removed bash script within main chaste source code as this should now be created by the user in their build directory.
Copy link

codecov bot commented Jun 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (e26ac25) to head (b3b2fad).
Report is 33 commits behind head on develop.

Current head b3b2fad differs from pull request most recent head 2d19b79

Please upload reports for the commit 2d19b79 to get more accurate results.

Additional details and impacted files
@@            Coverage Diff            @@
##           develop      #282   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files         1029      1029           
  Lines        51386     51383    -3     
=========================================
- Hits         51386     51383    -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mirams mirams changed the title Pull request for adding two new simple command line based tutorials. Pull request for adding simple command line based tutorial. Jun 11, 2024
@BJackal BJackal merged commit 7164e4b into develop Jul 2, 2024
31 checks passed
@mirams mirams deleted the 267-command-line-tutorial branch July 2, 2024 09:25
@mirams mirams restored the 267-command-line-tutorial branch July 2, 2024 09:25
@mirams
Copy link
Member

mirams commented Jul 2, 2024

Didn't mean to delete branch, we'll do that when ticket is closed!

We'll want to add (manually) an entry to the "Core Functionality" part of the menu page: https://chaste.github.io/docs/user-tutorials/

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.

Create user tutorial for cell-based simulation command line arguments and parameter sweeping
2 participants