UM-PEPL / HallThruster.jl

An open-source fluid Hall thruster code
Other
16 stars 9 forks source link

Restarts fix #127

Closed DecBrick closed 3 months ago

DecBrick commented 3 months ago

Three changes from this commit:

  1. Fixed the load restart to correctly load anomalous and dt values to fix syntax error with restarts. This allows restarts to be usable again
  2. Modified the write and read restarts to save the full params object rather than select fields to ensure all configuration information is saved. This change results is roughly a 0.05% increase in storage size per save but is more robust for saving the params object if future versions of the code modify the params object.
  3. Modified the restarts unit tests to create a restart rather than loading a saved test restart and add a test to try and start a simulation directly from a restart. This change should make the unit tests more robust such that syntax errors like the one fixed in this PR are caught earlier.
codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 86.86%. Comparing base (3209197) to head (11852c8).

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/UM-PEPL/HallThruster.jl/pull/127/graphs/tree.svg?width=650&height=150&src=pr&token=ADOlTglgOb&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=UM-PEPL)](https://app.codecov.io/gh/UM-PEPL/HallThruster.jl/pull/127?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=UM-PEPL) ```diff @@ Coverage Diff @@ ## main #127 +/- ## ========================================== + Coverage 83.62% 86.86% +3.23% ========================================== Files 35 35 Lines 1942 1941 -1 ========================================== + Hits 1624 1686 +62 + Misses 318 255 -63 ``` | [Files](https://app.codecov.io/gh/UM-PEPL/HallThruster.jl/pull/127?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=UM-PEPL) | Coverage Δ | | |---|---|---| | [src/simulation/restart.jl](https://app.codecov.io/gh/UM-PEPL/HallThruster.jl/pull/127?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=UM-PEPL#diff-c3JjL3NpbXVsYXRpb24vcmVzdGFydC5qbA==) | `91.07% <100.00%> (+91.07%)` | :arrow_up: | | [src/simulation/simulation.jl](https://app.codecov.io/gh/UM-PEPL/HallThruster.jl/pull/127?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=UM-PEPL#diff-c3JjL3NpbXVsYXRpb24vc2ltdWxhdGlvbi5qbA==) | `90.65% <100.00%> (+0.54%)` | :arrow_up: | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/UM-PEPL/HallThruster.jl/pull/127/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=UM-PEPL) ------ [Continue to review full report in Codecov by Sentry](https://app.codecov.io/gh/UM-PEPL/HallThruster.jl/pull/127?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=UM-PEPL). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=UM-PEPL) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://app.codecov.io/gh/UM-PEPL/HallThruster.jl/pull/127?dropdown=coverage&src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=UM-PEPL). Last update [3209197...11852c8](https://app.codecov.io/gh/UM-PEPL/HallThruster.jl/pull/127?dropdown=coverage&src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=UM-PEPL). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=UM-PEPL).
archermarx commented 3 months ago

Thanks for the PR. My main concern with saving the whole params struct is that if we change the params struct and then load an old solution, the params will be incompatible. We'd need some infrastructure for making all the params from old versions compatible with new ones

DecBrick commented 3 months ago

Looking into the loading in load_restarts, the only parts of the restart params that are actually loaded into the new simulation params are z_cell and ncharge. As most of the params are reset using the called configuration, rather that loaded from the old values, I don't forsee version conflicts really arising. The main benefit saving the entire params object offers is that one can access the old config for debugging

archermarx commented 3 months ago

Oh great. This looks good to me then! I'll tag a new patch version once this merges