Skip to content

BUG: Mark failed time steps when exporting#1471

Draft
pschultzendorff wants to merge 1 commit intodevelopfrom
1421-solutionstrategyafter_nonlinear_failure-saves-time-step-data-causing-weird-visualization
Draft

BUG: Mark failed time steps when exporting#1471
pschultzendorff wants to merge 1 commit intodevelopfrom
1421-solutionstrategyafter_nonlinear_failure-saves-time-step-data-causing-weird-visualization

Conversation

@pschultzendorff
Copy link
Contributor

@pschultzendorff pschultzendorff commented Aug 13, 2025

Proposed changes

Attempt at fixing #1421.

The idea is to have SolverStatistics keep track of whether the current nonlinear loop has converged. When calling DataSavingMixin.save_data_time_step, SolverStatistics.save stores the convergence flag in the JSON file. DataSavingMixin.write_pvd_and_vtu then prefixes exported files with failed_ in the case of non-convergence. Lastly, only the files corresponding to converged time steps are included in the data.pvd file for the full simulation.

The code works, except for the last part. To track failed time steps, I added Exporter._exported_timesteps_convergence, which masks non-converged steps before writing data.pvd. However, when DataSavingMixin.write_pvd_and_vtu calls Exporter.write_pvd, it passes TimeManager.exported_times for times and None for file_extension. The latter parameter is then replaced by Exporter._exported_timesteps. As far as I see, tracking exported time steps at two different places and coupling both in Exporter.write_pvd is prone to bugs.

Before I make this even more convoluted, we should discuss whether it is worth to clean this up and unify bookkeeping of exported time steps.

Also this is potentially a breaking change, as I removed SolutionStrategy.converge_status in favor of SolverStatistics.nl_is_converged. The former wasn't used anywhere though.

Types of changes

  • Minor change (e.g., dependency bumps, broken links).
  • Bugfix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as expected).
  • Testing (contribution related to testing of existing or new functionality).
  • Documentation (contribution related to adding, improving, or fixing documentation).
  • Maintenance (e.g., improve logic and performance, remove obsolete code).
  • Other:

Checklist

  • The documentation is up-to-date.
  • Static typing is included in the update.
  • This PR does not duplicate existing functionality.
  • The update is covered by the test suite (including tests added in the PR).
  • If new skipped tests have been introduced in this PR, pytest was run with the --run-skipped flag.

Tests run successfully, but exporting does not work correctly atm (2025-08-15)

@IvarStefansson
Copy link
Contributor

Let's reconsider this once #1448 is merged. Converting to draft in the meantime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR - in progress A PR that is currently a work in progress.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SolutionStrategy.after_nonlinear_failure saves time step data causing weird visualization

2 participants