Skip to content

Conversation

@wpbonelli
Copy link
Member

@wpbonelli wpbonelli commented Dec 14, 2025

Followup on #2665

Add an optional parameter replace_existing to set_all_data_external() methods, defaulting True. This guarantees that user settings will be respected when these methods are called, at the cost of rewriting when it may not be necessary. Previously settings were only respected when writing to a new workspace. So, now:

# this works now by default
sim.simulation_data.max_columns_of_data = 1
sim.set_all_data_external()
sim.write_simulation()  # Files have 1 column

# to avoid rewriting external files, this is now necessary
# where previously it would be lazy by default.
sim.set_all_data_external(replace_existing=False)

This decision prioritizes always respecting user settings. I could also see a case for defaulting False, so you need to opt into replacing existing files if you want newly modified column settings applied. This would prioritize IO performance over respecting settings. I'm not sure what the right choice is here, curious how others feel.

As mentioned in #2419, external files are written on set_all_data_external() instead of simply configured to be external, then only written on write_simulation(). In other words, due to architectural quirks:

sim.set_all_data_external()  # this writes external files
sim.simulation_data.max_columns_of_data = 1  # too late
sim.write_simulation()  # files still have old column count

This PR does nothing to address that. I have looked into a fix but it would be somewhat invasive so I'm tempted to punt to 4.x.

Add an optional parameter replace_existing to set_all_data_external() methods, defaulting True. This guarantees that user settings will be respected when these methods are called, at the cost of rewriting when it may not be necessary.
@codecov
Copy link

codecov bot commented Dec 14, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.7%. Comparing base (556c088) to head (2db02ee).
⚠️ Report is 96 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2666      +/-   ##
===========================================
+ Coverage     55.5%    72.7%   +17.1%     
===========================================
  Files          644      667      +23     
  Lines       124135   129516    +5381     
===========================================
+ Hits         68947    94164   +25217     
+ Misses       55188    35352   -19836     
Files with missing lines Coverage Δ
flopy/mf6/mfmodel.py 56.9% <ø> (-24.0%) ⬇️
flopy/mf6/mfpackage.py 68.4% <100.0%> (-13.2%) ⬇️
flopy/mf6/mfsimbase.py 62.6% <ø> (-12.8%) ⬇️

... and 556 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@wpbonelli wpbonelli marked this pull request as ready for review December 14, 2025 14:49
@wpbonelli
Copy link
Member Author

This decision prioritizes always respecting user settings. I could also see a case for defaulting False, so you need to opt into replacing existing files if you want newly modified column settings applied. This would prioritize IO performance over respecting settings. I'm not sure what the right choice is here, curious how others feel.

The test failing because of this makes me concerned people might also have scripts expecting the old behavior (not overwriting on set_all_data_external() unless it's a fresh workspace), and the new replace_existing should default False instead of True

This will mean you need to opt into overwriting for column settings to be applied

@wpbonelli
Copy link
Member Author

wpbonelli commented Dec 16, 2025

Changed the default to False. This preserves existing behavior, with the ability to opt in to the new behavior.

...
sim.set_all_data_external()
sim.write_simulation()  # Files have N columns

# to rewrite existing files with new column setting
sim.simulation_data.max_columns_of_data = 1
sim.set_all_data_external(replace_existing=True)

@wpbonelli wpbonelli merged commit 3e1b7a9 into modflowpy:develop Dec 16, 2025
20 checks passed
@wpbonelli wpbonelli deleted the more-2420 branch December 16, 2025 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant