Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include core constants as an attribute of each model constants class #342

Closed
jacobcook1995 opened this issue Nov 21, 2023 · 2 comments
Closed

Comments

@jacobcook1995
Copy link
Collaborator

jacobcook1995 commented Nov 21, 2023

Is your feature request related to a problem? Please describe.
It's fiddly to have to track both model constants and core constants data-classes separately. So it would be simpler for each model constants data class to contain every relevant constant. By including the core constants data class as an attribute we can also retain the advantage of having a single location where all core constants are defined.

Describe the solution you'd like
Core constants could be set as an attribute and relevant constants could be returned using properties, e.g.

class LitterConsts:
    ...
    core_constants: CoreConsts = CoreConsts()

    @property
    def depth_of_active_soil_layer(self) -> float:
        return core_constants.depth_of_active_soil_layer

Then when constants data classes are created from the Config, the CoreConsts would have to be created first, and used to created the model specific constants data classes.

Describe alternatives you've considered
I don't see a clear alternative aside just putting up with each model having a separate core_constants and a model_constants dataclass.

Additional context
This thread contains discussion of the issue. Also this issue has overlap with the broader issue #208.

@davidorme
Copy link
Collaborator

So it would be simpler for each model constants data class to contain every relevant constant.

Simpler for each model constants data class to provide an indirect link through to centrally configured core constants that are needed for this model.

I think this is probably cleaner than passing two constants classes and having to remember which goes where. But... I'm not sure how this plays out against the possibility of having more than one model constants class. People can do that and it might make sense - likely to be important for plants, certainly. I guess then you still have two constants classes in the model __init__ but one of them is picked to also handle passing CoreConsts? The issue is then that constants_loader needs to know which ConstantsClasses need to be passed the CoreConsts instance.

I definitely think this looks cleaner - and I can see how remembering which const comes from where is a pain - but it is also more complex.

@davidorme
Copy link
Collaborator

Closing this. Over in #364, we've found a clean solution to implement this, but at the moment have chosen to keep separate CoreConsts and ModelConsts namespaces, probably with the addition of making a single CoreConsts instance at startup to use across all models.

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

No branches or pull requests

2 participants