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

Potential desync in model configuration at the model.py level #608

Closed
1 of 3 tasks
irm-codebase opened this issue Jun 19, 2024 · 1 comment · Fixed by #639
Closed
1 of 3 tasks

Potential desync in model configuration at the model.py level #608

irm-codebase opened this issue Jun 19, 2024 · 1 comment · Fixed by #639
Assignees
Labels
bug v0.7 (upcoming) version 0.7
Milestone

Comments

@irm-codebase
Copy link
Contributor

irm-codebase commented Jun 19, 2024

What happened?

Calliope v0.7 keeps most of its attributes within model._model_data.attrs[] in the form of dictionaries.

However, model math and config are replicated in model.math and model.config. This introduces potential sync issues, necessitating some helper functions to keep these up-to-date. This is done via the _add_observed_dict() and _add_model_data_methods().

But this requires careful use of these functions in several parts of the code, and is a bit inefficient (we essentially duplicate the same model property).

Which operating systems have you used?

  • macOS
  • Windows
  • Linux

Version

v0.7

Relevant log output

No response

@irm-codebase
Copy link
Contributor Author

irm-codebase commented Jun 19, 2024

As a proposal, I believe we should extend our use of @properties to standardize access to math and config.

I think this should do the trick:

@property
    def name(self):
        return self._model_data.attrs["name"]

    @name.setter
    def name(self, value: str):
        self._model_data.attrs["name"] = value

    @property
    def inputs(self):
        return self._model_data.filter_by_attrs(is_result=0)

    @property
    def results(self):
        return self._model_data.filter_by_attrs(is_result=1)

    @property
    def math(self):
        value = self.self._model_data.attrs["math"]
        if not isinstance(value, AttrDict):
            value = AttrDict(value)
        return value

    @math.setter
    def math(self, value: dict):
        if not isinstance(value, dict):
            raise TypeError(
                f"Attempted to add dictionary math to model, but received argument of type `{type(value).__name__}`"
            )
        self._model_data.attrs["math"] = AttrDict(value)

    @math.deleter
    def math(self):
        del self._model_data.attrs["math"]

    @property
    def config(self):
        value = self._model_data.attrs["config"]
        if not isinstance(value, AttrDict):
            value = AttrDict(value)
        return value

    @config.setter
    def config(self, value: dict):
        if not isinstance(value, dict):
            raise TypeError(
                f"Attempted to add configuration to model, but received argument of type `{type(value).__name__}`"
            )
        self._model_data.attrs["config"] = value

    @config.deleter
    def config(self):
        del self._model_data.attrs["config"]

    @property
    def is_built(self):
        return self._is_built

    @property
    def is_solved(self):
        return self._is_solved

@irm-codebase irm-codebase self-assigned this Jun 19, 2024
@irm-codebase irm-codebase changed the title Potential desync in model configuration Potential desync in model configuration at the model.py level Jun 25, 2024
@irm-codebase irm-codebase added the v0.7 (upcoming) version 0.7 label Jul 12, 2024
@irm-codebase irm-codebase mentioned this issue Jul 16, 2024
4 tasks
@sjpfenninger sjpfenninger added this to the 0.7.0 milestone Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment