Skip to content

Fix reset/init behaviors#168

Merged
ffelten merged 7 commits into
mainfrom
chore/no_reset_default
Sep 2, 2025
Merged

Fix reset/init behaviors#168
ffelten merged 7 commits into
mainfrom
chore/no_reset_default

Conversation

@ffelten
Copy link
Copy Markdown
Collaborator

@ffelten ffelten commented Aug 27, 2025

Description

Fixes #158. This should fix weird behaviors, like users seeing study_None/ generated when instantiate the airfoil problem. The folders are now generated when calling simulate or optimize in airfoil.

The API now sets the seed at initialization and provides a way to cleanup/reset whenever you want (which you should do in between simulations/optimizations). Pinging @arthurdrake1 on this because he resolved an issue in Beams with that; Arthur does the current API (API 4 in the comments below) work for you?

Type of change

Please delete options that are not relevant.

  • Documentation only change (no code changed)
  • Bug fix (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)
  • This change requires a documentation update

Checklist:

  • I have run the pre-commit checks with pre-commit run --all-files
  • I have run ruff check . and ruff format
  • I have run mypy .
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@ffelten ffelten requested a review from g-braeunlich August 27, 2025 13:53
Copy link
Copy Markdown
Collaborator

@g-braeunlich g-braeunlich left a comment

Choose a reason for hiding this comment

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

Do you really want to enforce that?
Seems like a downgrade of user-friendliness to me.
In e.g. numpy you also dont have to call seed when using a random number generator.
An other example would be jax, where afaik you always need a to provide a seed ("key"), but there, the key is supplied as an argument to the constructor (to prevent violating the invariant) and they also dont require you to call another method.

@markfuge
Copy link
Copy Markdown
Member

I agree with @g-braeunlich here that this is a big downgrade in userfriendliness, and I would in general prefer if the user never had to think about or worry about explicitly resetting anything.

Are you sure you have investigated alternative options for addressing this behavior across problems, even if it means more complex back end checks on our side? I would only want to mandate using reset as a last resort.

@ffelten
Copy link
Copy Markdown
Collaborator Author

ffelten commented Aug 28, 2025

[I am thinking out loud on this. I do not have a super strong opinion. Just laying out why I chose to go for this solution]

Hello there,

So, we had reset being called implicitly until now. Several people have surfaced that it did "too much magic behind.", .e.g., why is Airfoil creating a study_None/ folder when I instantiate the object? I guess we are pointing at a tradeoff of magic vs. control--calling it magic because sometimes it is not user-friendly, i.e., in the current state.

For instance, in Beams2D, we had problems because the internal state was not properly reset between calls to optimize or simulate. This ultimately led to wrong results. We could do that automatically:

  • after optimization/simulate: what if as a user I want to inspect that internal state for some reason?
  • before optimization/simulate: I guess it is okay if we pass the seed as an argument to both methods. Also note that optimize calls simulate in a loop, so you'll need another argument telling simulate to not call reset in this case.

Now, in Airfoil, reset is creating the folders (study_{seed}) where we later put intermediary files (meshes, logs, optimization history), completed template files, etc. It also allows you to clean these folders up if you want, so if you call reset(cleanup=True), it will delete the folder of the previous study and create new ones for the current study. (Btw, heatconduction is not consistent with this #46.)

Seems like a good idea for users to have control over whether they want to clean up their filesystem or keep everything.
Again, doable automatically if we add yet another (unrelated) parameter to optimize/simulate?

My fears are essentially:

  • loss of control on the user side (things happen, I do not know why).
  • expansion of parameter in simulate or optimize, which are unrelated to those.

I honestly would love to discuss this and see if there is a way I do not see yet?


For the comparison with Numpy; it used to be more "magic" and is now reverting to more control. Currently, it explicitly asks you to "create" (or reset 😉) the RNG, and then pass it around as a parameter in every method, or have every method/object have its own RNG. This way, you know where your RNG state is (it is not hidden inside a (global) object somewhere).

Jax is a different story, as they propose a purely functional API and push for no internal mutable object state (for easy concurrency, among other things), so you are passing everything (i.e., the RNG key) as a parameter to every function.

@g-braeunlich
Copy link
Copy Markdown
Collaborator

A couple of the problems, you mention before sound to me like the seed is coupled too much to other pieces of logic (i.e. output file name). I guess the study_None/ problem could be addressed independently - there the magic is not really related to the seed.

As for the Beams2D problem: here I dont have a good enough understanding of the codebase to say whether the problem could also be decoupled or not, but I also would be optimistic that another solution could be found.

In any case:
If you want to enforce a seed, then I would suggest the jax way: make it a required argument of the constructor => no need for an additional call to a method / no check required if reset has been called.

@ffelten
Copy link
Copy Markdown
Collaborator Author

ffelten commented Aug 28, 2025

A couple of the problems, you mention before sound to me like the seed is coupled too much to other pieces of logic (i.e. output file name). I guess the study_None/ problem could be addressed independently - there the magic is not really related to the seed.
As for the Beams2D problem: here I dont have a good enough understanding of the codebase to say whether the problem could also be decoupled or not, but I also would be optimistic that another solution could be found.

I agree there are separate concerns addressed in reset:

  • seed for reproducibility, e.g., make random_design() calls reproducible --- this is a core feature.
  • reset internal states (class members or files) --- this I think we can manage to automatically reset.
  • naming files: having folders named with study_{seed}/. I really think this is useful; at any moment, I know exactly which folder contain the information I want, and it is easily readable/trackable for a human. This is also probably helpful for parallel slurm jobs (avoid having multiple jobs write in the same folder).

This is not good. And I agree we should clarify what the intent of reset is.

In any case: If you want to enforce a seed, then I would suggest the jax way: make it a required argument of the constructor => no need for an additional call to a method / no check required if reset has been called.

Now you've got me thinking. There are several possible APIs I see.

1. Seed in constructor, folders named with seed

problem = Airfoil(seed=1)
design, _ = problem.random_design()
opt_design, history = problem.optimize(design) # creates `study_1/`
problem = Airfoil(seed=2)
obj = problem.simulate(design) # creates `study_2/`

# Now how do I clean my mess up?

2. Seed in constructor, folders named with something else, e.g., timestamp

problem = Airfoil(seed=1)
design, _ = problem.random_design()
opt_design, history = problem.optimize(design) # creates `study_1_YYYY_MM_DD_12:53/`
obj = problem.simulate(design) # creates `study_1_YYYY_MM_DD_12:54/`

# Same, how do I clean this up?

3. Seed in reset (current proposal)

Note that this is the same as 1, except you're calling reset instead of the constructor.

problem = Airfoil()
problem.reset(seed=1)
design, _ = problem.random_design()
opt_design, history = problem.optimize(design) # creates `study_1/`
problem.reset(seed=2, cleanup=True) # deletes `study_1/`
obj = problem.simulate(design) # creates `study_2/`

@g-braeunlich
Copy link
Copy Markdown
Collaborator

Just to be sure that you understand, why I really dont like 3.:
It allows an uninitialized (violated invariant) intermediate state which easily allows users to fall into trap and raise an exception.
IMHO such states should be avoided as much as possible.

@ffelten
Copy link
Copy Markdown
Collaborator Author

ffelten commented Aug 28, 2025

4

problem = Airfoil(seed=1)
design, _ = problem.random_design()
opt_design, history = problem.optimize(design) # creates `study_1/`
problem.reset(seed=2, cleanup=True) # deletes `study_1/`
obj = problem.simulate(design) # creates `study_2/`

@ffelten ffelten requested a review from g-braeunlich September 2, 2025 08:00
@ffelten ffelten changed the title User has to call reset explicitly Fix reset/init behaviors Sep 2, 2025
Copy link
Copy Markdown
Collaborator

@g-braeunlich g-braeunlich left a comment

Choose a reason for hiding this comment

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

👍

@arthurdrake1
Copy link
Copy Markdown
Contributor

@ffelten API 4 looks good to me. I've tested the same loop as before and everything appears to work fine.

@ffelten ffelten merged commit a741531 into main Sep 2, 2025
13 checks passed
@ffelten ffelten deleted the chore/no_reset_default branch September 2, 2025 13:10
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.

Reset should be called explicitly by users

4 participants