ert icon indicating copy to clipboard operation
ert copied to clipboard

dumb down base run model

Open valentin-krasontovitsch opened this issue 2 years ago • 3 comments

i'm adding an argument to the from_legacy method of the ensemble builder, which explodes the pylint (6 arguments is just one too many).

looking at the argument list, it seems like providing a queue config and analysis config next to a res config seems excessive, considering that probably res config contains the other two.

checking out where that method is used, we move to base run model, which builds ensembles to run an ensemble evaluator. it gets a queue config passed during init, and never changes it. cool. it also gets an enkf main, which it uses to retrieve res config and analysis config. maybe it doesn't need to get a queue config, but can retrieve that one from the enkf main object, too?

and from legacy doesn't need to get an analysis config, as it can access it from the res config!

but might the queue config passed to base run model be different from the queue config found in the enkf main object?

checking further where base run model gets its res config and queue config, i find that it is a base class. classes like EnsembleSmoother build on it.

looking at model_factory.py, where (instances of) some of these classes get instantiated, I see that indeed they get passed a variable ert as the enkf main argument, and ert.get_queue_config() as the queue config argument.

now certainly more grepping and checking needs to be done, but it seems like the arguments here can be simplified.

I reiterate that the motivation for this is pylint complaining about too many arguments.

if you think i should just put a comment to tell pylint to behave, i'll be OK with that, too.

@oyvindeide, based on related issues it seems like you might be able to chime in here?

valentin-krasontovitsch avatar Oct 04 '22 18:10 valentin-krasontovitsch

There was a bit to unpack in the issue, so not sure I am answering correctly, but in general we have tried to make the signatures a bit more explicit, and avoid the unpacking from EnKFMain, and passing explicitly the configs that are required to get better separation between different parts of the code base.

oyvindeide avatar Oct 05 '22 12:10 oyvindeide

I see, cool! that makes sense. thanks for explaining, and I'm sorry about the wall of text :sweat_smile:

So in that case I guess we don't really mind much (at least for now) about pylint complaining, and I'm gonna use an ignore clause. thanks again for your help!

valentin-krasontovitsch avatar Oct 05 '22 13:10 valentin-krasontovitsch

#4064 moves from_legacy to BaseRunModel which gets rid of most of the arguments as they were members of BaseRunModel . In general, this seems to have been a case of feature-envy.

eivindjahren avatar Oct 20 '22 12:10 eivindjahren