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

Command line options moved to input file #262

Merged
merged 22 commits into from
May 17, 2023

Conversation

willGraham01
Copy link
Collaborator

@willGraham01 willGraham01 commented Apr 3, 2023

Closes #261, closes #217 | Hence also closes #208

Important files with changes

Moves command-line flags to the input file

  • ArgumentParser no longer reads the -c and -fd flags
  • The option to pass these flags via the command line has been removed, and the CLI -h help does not say anything about them
  • InputFlags class created to handle reading (possibly non-existent) names from the input files. This is a counterpart to InputMatrices, which requires that all the datastructures it searches for in the input file exist.
  • Simplifies some of the initialisation methods for SimulationManager and ObjectsFromInfile
  • Removes duplicate InterpolationMethods enum
  • Namespaces flag-related variables and enums. Flag-related enums are weakly typed to bools indicating whether or not they are on/off
  • usecd has been replaced with use_pstd and intmethod has been replaced with use_bli as per Names of flag variables in the input file #261

Updates system tests accordingly

  • There is another Zenodo update required for the system test data, since the config files no longer need to provide command-line options to TDMS and the input files correspondingly need to provide this information.
  • Portions of code that add ow-redundant flags to TDMS executable calls have been removed.

@samcunliffe
Copy link
Member

As discussed in our meeting 2023-04-13, when this guy is merged we will tag v1.0.0 (following semver it can't be v0.anything).

Conveniently, this would also coincide with a stable trustworthy version.

@samcunliffe
Copy link
Member

Just a note for when we get here. There are some slightly different lines to be deleted in the README now. And a little bit of the unit-tests can be deleted with this merge.

@willGraham01 willGraham01 force-pushed the wgraham-217-update_ifdtdmatrix branch from 3c22e76 to 2a9b38f Compare May 4, 2023 15:14
Base automatically changed from wgraham-217-update_ifdtdmatrix to main May 5, 2023 09:30
@willGraham01 willGraham01 force-pushed the wgraham-217-CLI_to_input_file branch 3 times, most recently from 7efd9d5 to 4e78084 Compare May 5, 2023 09:51
@willGraham01 willGraham01 self-assigned this May 5, 2023
@willGraham01 willGraham01 added enhancement New feature or request priority:1 Top priority review required A review approval is required before merging subtask A subtask of a parent issue labels May 5, 2023
@codecov
Copy link

codecov bot commented May 5, 2023

Codecov Report

Patch coverage: 45% and project coverage change: -1 ⚠️

Comparison is base (bf6eaeb) 48% compared to head (40df6da) 48%.

❗ Current head 40df6da differs from pull request most recent head b5ad930. Consider uploading reports for the commit b5ad930 to get more accurate results

Additional details and impacted files
@@         Coverage Diff         @@
##           main   #262   +/-   ##
===================================
- Coverage    48%    48%   -1%     
===================================
  Files        63     65    +2     
  Lines      7826   7792   -34     
===================================
- Hits       3783   3712   -71     
- Misses     4043   4080   +37     
Impacted Files Coverage Δ
tdms/include/field.h 0% <0%> (ø)
tdms/include/input_flags.h 0% <0%> (ø)
tdms/include/interpolation_methods.h 0% <ø> (ø)
tdms/include/output_matrices/output_matrices.h 0% <0%> (ø)
...s/include/simulation_manager/objects_from_infile.h 0% <ø> (ø)
...ms/include/simulation_manager/simulation_manager.h 0% <ø> (ø)
tdms/include/simulation_parameters.h 100% <ø> (ø)
tdms/src/argument_parser.cpp 74% <ø> (+6%) ⬆️
tdms/src/simulation_manager/execute_simulation.cpp 5% <ø> (-<1%) ⬇️
...src/simulation_manager/execute_update_Ex_split.cpp 35% <ø> (-1%) ⬇️
... and 8 more

... and 19 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

README.md Outdated Show resolved Hide resolved
tdms/include/input_flags.h Outdated Show resolved Hide resolved
tdms/include/input_flags.h Outdated Show resolved Hide resolved
tdms/include/output_matrices/output_matrices.h Outdated Show resolved Hide resolved
@samcunliffe
Copy link
Member

Patch coverage: 58% and no project coverage change.

So why did you fail?

codecov/project — 48% (-0%) compared to b510b06

Copy link
Member

@samcunliffe samcunliffe left a comment

Choose a reason for hiding this comment

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

Nice. Sorry for being slow.

A few docs suggestions, and I'd plop some of the implementation stuff into a .cpp file rather than the header.

Whats with 6?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
doc/developers.md Show resolved Hide resolved
doc/developers.md Outdated Show resolved Hide resolved
tdms/include/field.h Outdated Show resolved Hide resolved
tdms/include/input_flags.h Outdated Show resolved Hide resolved
@samcunliffe
Copy link
Member

Requesting changes explicitly because: automerge is on.

@willGraham01 willGraham01 force-pushed the wgraham-217-CLI_to_input_file branch from 40df6da to 5d30807 Compare May 17, 2023 13:20
Copy link
Member

@samcunliffe samcunliffe left a comment

Choose a reason for hiding this comment

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

🍉

@samcunliffe samcunliffe disabled auto-merge May 17, 2023 13:51
@samcunliffe samcunliffe added this pull request to the merge queue May 17, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 17, 2023
@willGraham01 willGraham01 added this pull request to the merge queue May 17, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks May 17, 2023
@samcunliffe samcunliffe added this pull request to the merge queue May 17, 2023
@samcunliffe
Copy link
Member

I doubled the merge queue runtime max. So it's now 120 mins (for instances where we need to flush the cache this makes sense and should be in the minority).

If this still doesn't work, then we revert IMHO.

Merged via the queue into main with commit e3d81f2 May 17, 2023
10 checks passed
@samcunliffe
Copy link
Member

🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority:1 Top priority review required A review approval is required before merging subtask A subtask of a parent issue
Projects
None yet
2 participants