Skip to content

Conversation

tobyallwood
Copy link
Collaborator

TopoStats Pull Requests

Changed the main output CSV names:

  • all_statistics.csv -> grain_statistics.csv
  • image_stats.csv -> image_statistics.csv
  • all_mol_stats -> molecule_statistics.csv
  • all_disordered_segment_stats -> branch_statistics.csv

A new option in config has also been added called output_stats_file which can be set to full or basic. full will cause all four csvs to output and basic will ignore molecule_statistics.csv and branch_statistics.csv and only output grain_statistics.csv and image_statistics.csv.


Before submitting a Pull Request please check the following.

  • Existing tests pass.
  • Documentation has been updated and builds. Remember to update as required...
    • docs/configuration.md
    • docs/usage.md
    • docs/data_dictionary.md
    • docs/advanced.md and new pages it should link to.
  • Pre-commit checks pass.

topostats/default_config.yaml

If adding options to topostats/default_config.yaml please ensure.

  • There is a comment adjacent to the option explaining what it is and the valid values.
  • A check is made in topostats/validation.py to ensure entries are valid.
  • Add the option to the relevant sub-parser in topostats/entry_point.py.

Copy link
Collaborator

@ns-rse ns-rse left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the output files here @tobyallwood

I think the output_stats_file could be misleading (as I've also noted in the comments below) as reading that makes me think that its a file-name parameter that the statistics are written to. Dropping the suffix _file would leave output_stats which is more representative in my view (it could just be me though).

Copy link
Collaborator

@ns-rse ns-rse left a comment

Choose a reason for hiding this comment

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

Thanks for going through these @tobyallwood

I've marked the previous conversations as "Resolved" but spotted a few minor things that have been introduced (comments inline).

If you make these changes locally then you can avoid making another "Addressing PR feedback" commit by using git commit --amend and then, because no one else is likely to have touched this branch on GitHub or anywhere else you can git push --force to force push the changes to the branch on GitHub and the pull request will be updated (this helps keep the history cleaner and avoids lots of small commits, if you want to read more on this the Git With It material I wrote and occassionally deliver has details).

| `log_level` | | string | `info` | Verbosity of logging, options are (in increasing order) `warning`, `error`, `info`, `debug`. |
| `cores` | | integer | `2` | Number of cores to run parallel processes on. |
| `file_ext` | | string | `.spm` | File extensions to search for. |
| `output_stats` | | string | `full` | statistics to write to `.csv` files, options are `full` (all information on grains as well as branch and molecule statistics) or `basic` (just grain level statistics) |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing The amount of

Suggested change
| `output_stats` | | string | `full` | statistics to write to `.csv` files, options are `full` (all information on grains as well as branch and molecule statistics) or `basic` (just grain level statistics) |
| `output_stats` | | string | `full` | The amount of statistics to write to `.csv` files, options are `full` (all information on grains as well as branch and molecule statistics) or `basic` (just grain level statistics) |

@tobyallwood tobyallwood force-pushed the tobyallwood/csv-names branch from 415f372 to d2c7207 Compare October 13, 2025 09:37
Copy link
Collaborator

@ns-rse ns-rse left a comment

Choose a reason for hiding this comment

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

Thanks @tobyallwood, hopefully the nomenclature will stick going forward.

This can be merged now 👍

@tobyallwood tobyallwood enabled auto-merge October 15, 2025 14:10
Copy link
Collaborator

@SylviaWhittle SylviaWhittle left a comment

Choose a reason for hiding this comment

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

:)

@tobyallwood tobyallwood added this pull request to the merge queue Oct 15, 2025
Merged via the queue into main with commit df2e2c3 Oct 15, 2025
11 of 16 checks passed
@tobyallwood tobyallwood deleted the tobyallwood/csv-names branch October 15, 2025 14:11
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.

3 participants