Skip to content

Expose Run.is_default in facade#153

Merged
glatterf42 merged 3 commits into
mainfrom
enh/expose-run-is-default
Apr 29, 2025
Merged

Expose Run.is_default in facade#153
glatterf42 merged 3 commits into
mainfrom
enh/expose-run-is-default

Conversation

@glatterf42

Copy link
Copy Markdown
Member

Working on iiasa/ixmp#552, I realized that ixmp4.core.Run should expose its is_default value, which is already present in run._model.
This PR exposes the value by using a function (and tests that calling set_as_default() and unset_as_default() update is_default()). It could also be a property, though it's more dynamic than e.g. Run.id or Run.model, that's why I opted for a function: to indicate that this value needs to be retrieved when it's requested.

@glatterf42 glatterf42 added the enhancement New feature or request label Jan 24, 2025
@glatterf42 glatterf42 requested a review from meksor January 24, 2025 12:57
@glatterf42 glatterf42 self-assigned this Jan 24, 2025
@codecov

codecov Bot commented Jan 24, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.9%. Comparing base (c8b231c) to head (64b32ca).
⚠️ Report is 20 commits behind head on main.

Additional details and impacted files
@@          Coverage Diff          @@
##            main    #153   +/-   ##
=====================================
  Coverage   88.9%   88.9%           
=====================================
  Files        236     236           
  Lines       8656    8661    +5     
=====================================
+ Hits        7698    7703    +5     
  Misses       958     958           
Files with missing lines Coverage Δ
ixmp4/core/run.py 100.0% <100.0%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@glatterf42 glatterf42 force-pushed the enh/run-clone branch 3 times, most recently from c0d84c8 to ed0a184 Compare January 30, 2025 10:43
@glatterf42 glatterf42 changed the base branch from enh/run-clone to enh/remove-superfluous-ellipses January 30, 2025 11:00
@glatterf42 glatterf42 force-pushed the enh/expose-run-is-default branch from 78a5825 to 71c0e69 Compare January 30, 2025 11:01
@meksor

meksor commented Apr 16, 2025

Copy link
Copy Markdown
Contributor

It's probably best to make this a @property like the rest of the attributes?

Comment thread ixmp4/core/run.py
def meta(self, meta: dict[str, PrimitiveTypes | np.generic | None]) -> None:
self._meta._set(meta)

def is_default(self) -> bool:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def is_default(self) -> bool:
@property
def is_default(self) -> bool:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, we can do that :)
Would we want this to be a read-only property or should this get a setter and deleter (that would then presumably call Run.set_as_default() and Run.unset_as_default())?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've not written setter and deleter for now since they are not required for the message_ix tutorials. If we want them at a later point, I propose a follow-up issue/PR.

We would likely want something of this sort:

@is_default.setter
def is_default(self, value: bool) -> None:
    if value:
        self.set_as_default()
    else:
        self.unset_as_default()

If it bothers us that mypy deems one test case as unreachable, we might want to set up an attribute _is_default and use is_default.setter to set that directly. Though even if is_default.getter simply returns the state of _is_default, mypy might not recognize this properly because it's hidden behind this extra layer.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree lets leave that for another time...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe @danielhuppmann can clarify whether we actually want that :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think doing is_default as a property is a good idea, but I would stick with the more "functional" set_as_default() to change the status, to avoid confusion or making changes by accident.

@glatterf42 glatterf42 force-pushed the enh/remove-superfluous-ellipses branch from 3cc17d2 to 8fe4cac Compare April 29, 2025 08:23
Base automatically changed from enh/remove-superfluous-ellipses to main April 29, 2025 09:23
@glatterf42 glatterf42 force-pushed the enh/expose-run-is-default branch from 71c0e69 to 64b32ca Compare April 29, 2025 09:39
@glatterf42 glatterf42 merged commit b0afc7c into main Apr 29, 2025
@glatterf42 glatterf42 deleted the enh/expose-run-is-default branch April 29, 2025 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants