Comments on State Logic

[musslick]

This is a thread to collect comments/suggestions on using the latest state logic in https://github.com/AutoResearch/autora-core/tree/feat/mini-hackathon/docs/cycle

[musslick]

FEATURE REQUEST: from autora.state.bundled import StandardState -- What does bundled mean? Would be more intuitive to import StatdardState directly from autora.state.

[musslick]

This should be fixed with the new refactoring.
We renamed
autora.state.bundled -> autora.state.standard and
autora.state.delta -> autora.state.init

[hollandjg]

I think it would be good to keep the delta stuff as its own sub package for now. What we could do is say:

• If you want fundamental things with the State/Delta mechanism, those can be imported from autora.state.delta,
• if you want the "standard" state things like the StandardState and wrappers state_fn_from_estimator..., then you can import those from autora.state.standard

(moving the things from delta to __init__ can't work, unfortunately, as that would break the namespace-package structure and make it more difficult to add a new way of doing things to the autora.state sub package in future)

[musslick]

I think the term "delta" is foreign to most users and importing state from it does not feel intuitive. I would strongly advocate for something simpler "state". We should discuss this.

[hollandjg]

@musslick , @younesStrittmatter and I talked about:

• merge all the state things we still use into a single file state.py feat!: update experimentalists grid_pool, random_pool, random_sample to use the new State mechanism autora-core#33
• delete the old state objects (Snapshot, History) – those are only used by the old workflow logic which needs completely rewriting. feat!: update experimentalists grid_pool, random_pool, random_sample to use the new State mechanism autora-core#33
• Update current version of autora-workflow to specify aurora-core < 3.0.0 chore: specify autora-core versions < 4 autora-workflow#38

[musslick]

FEATURE REQUEST: append logic for VariablesCollection

[hollandjg]

I'm not sure how this would work. The first example we made where we needed this was actually a deletion – how would you want the interface to look to deal with deletions, replacements and additions?

[musslick]

Let's make this lower priority for now.
I'm worried that the + operator is not intuitive to deletion. It should be an addition by default.

[musslick]

FEATURE REQUEST: Be able to declare input variables in the wrapper function

[hollandjg]

How do you mean?

[musslick]

E.g., specify not only the outputs but the inputs when calling on_state. Tagging @younesStrittmatter here who came up with the suggestion.

[musslick]

FEATURE REQUEST: when creating new state, make it more convenient to add fields to the state. It's inconvenient now because you have to create a new state class.

[hollandjg]

This isn't possible using dataclasses as far as I know – you always have to create a new state class.
What someone could do is implement the same logic but using a UserDict – they would have to work out how to translate what we've done for data classes so that it works on a dictionary, but that would be totally doable.

[hollandjg]

This needs to be a separate issue

[musslick]

Sounds good, also tagging @younesStrittmatter here who raised this issue.

[musslick]

BUG: model list problem @younesStrittmatter

[hollandjg]

Please can someone add a bit more info here? We can turn this into an issue.

[younesStrittmatter]

The issue is: When creating a custom State with fields as lists, then the Delta Object needs to return a list as well:
class ExtendedState(StandardState):
models_bms: List[BaseEstimator] = field(
default_factory=list,
metadata={"delta": "extend"},
)

@on_state()
def bms_theorist(experiment_data: pd.DataFrame, variables: VariableCollection, **kwargs):
ivs = [v.name for v in variables.independent_variables]
dvs = [v.name for v in variables.dependent_variables]
X, y = experiment_data[ivs], experiment_data[dvs]
new_model = BMSRegressor(epochs=EPOCHS).set_params(**kwargs).fit(X, y)
return Delta(models_bms=[new_model])

[younesStrittmatter]

The confusion came from the option to return a model (without the s) with the StandardState that is not a list because it has an alias to do so. So long story short: This might be okay as is and doesn't need to be an issue.

[musslick]

enforce that return Delta has to be a list

[musslick]

make sure to mention this in documentation

[chadcwilliams]

FEATURE REQUEST:

from autora.state.bundled import StandardState
from autora.state.delta import on_state
from autora.state.wrapper import state_fn_from_estimator

Definitely not a problem, but I find it odd that these all come from different places, especially the latter two as they do the same thing (conceptually).

It'd be nice to be able to:
from autora.state.bundled import StandardState, on_state, state_fn_from_estimator

[hollandjg]

I've made an update to feat: new general wrapper for State mechanism so that you can do

from autora.state.standard import StandardState, on_state, state_fn_from_estimator

[musslick]

Strongly agree with chad! Moving these to standard may help here and should resolve the issue about naming above.

[chadcwilliams]

Thanks for doing that John!

[hollandjg]

Another update: as discussed, I've simplified it again and now it looks like:

from autora.state import StandardState, on_state, state_fn_from_estimator

– everything is in one file now.

[hollandjg]

Hi all, I've updated AutoResearch/autora-core#33 with all the key changes from the mini hackathon.

Please note: I didn't include the align_dataframe_to_ivs function from https://github.com/AutoResearch/autora-core/blob/mini-hackaton-before-refactor-state/src/autora/utils/conversion.py because I don't think this should be part of the on_state function. There's an explanation here: AutoResearch/autora-core#33 (comment)

I would like to suggest that we include any additional features or requests as separate PRs. (However, if anything is broken with the existing PR, we'll update that.)

[musslick]

FEATURE REQUEST: some logic to combine outputs from multiple components into one output. E.g., current problem is that when two experimentalists are chained; the second one overwrites the first. We want some functionality that combines the output of both experimentalists @chadcwilliams

[younesStrittmatter]

*first experimentalist in each cycle

[chadcwilliams]

Oh, weird, I didn't realize it used the previous cycles conditions. But regardless, I'm happy with any implementation.

[hollandjg]

This probably needs more discussion, and potentially an exploration of the best way to chain experimentalists.

The reason that the we replace the conditions rather than appending to them is that we said "it's weird to append conditions, each time round you actually create some new ones and only use the newest ones, and so the conditions need to be replaced." This can be a thing for the next major version – updating the way we handle this in the next major version.

[musslick]

should cover this in the tutorial

[musslick]

potentially add some functionality to ease tutorial

[musslick]

DECISION: Have experimentalists & runners accept pandas df, (+numpy) and return pandas df