Repository navigation
Conversation
integrator, optimizer, and cell_filter
There was a problem hiding this comment.
Thanks for raising this!
From the torch-sim side, I'd prefer not to make these arguments accept str, since it weakens type checking: typos would only be caught at runtime.
Since Integrator, Optimizer, and CellFilter are StrEnums, converting from a string is already a one-liner.
I also noticed that atomate2 already does this conversion for optimizer in TorchSimOptimizeMaker.make (src/atomate2/torchsim/core.py).
Could you consider adding the same conversion in atomate2 for integrator and cell_filter instead of patching torch-sim?
To be clear, I understand this PR would save downstream packages a conversion step, but I think keeping the torch-sim API typed is worth that small cost.
|
Thanks for the feedback. I have no problem moving this to atomate2 if you prefer keeping stricter type hints |
|
The fix was moved to materialsproject/atomate2#1619 so I'm closing this PR. |
|
Thanks for your understanding! If the atomate2 side sees it differently, please feel free to ping me again! |
Summary
ts.integrate,ts.optimizeandts.get_cell_filter(and therefore thecell_filterargument of thefire/gradient_descent/lbfgs/bfgsoptimizers) now accept plain strings such as"nve","fire"or"frechet". Strings are converted withIntegrator(x)/Optimizer(x)/CellFilter(x)before the existingisinstancecheck. Enum members and(init_fn, step_fn)tuples behave exactly as before.An unknown string is not converted and falls through to the existing error branch, so the error message is the same as for any other invalid input.
Motivation
Jobflow jobs and atomate2 makers are stored as JSON documents. Enum members and callables round-trip poorly, whereas strings are trivially serialized, stored and queried. With this change a maker can simply hold, e.g.,
optimizer="fire",cell_filter="frechet",integrator="nvt_langevin". This leads to:Moreover, the change is minimal and does not require additional maintenance since the new conditional branch will not fail before reaching the already existing ones (1 line below the new code block).
Checklist
Before a pull request can be merged, the following items must be checked: