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

Proposed change to the __repr__ method of the Derivatives class #348

Closed
scott-huberty opened this issue Feb 28, 2024 · 1 comment · Fixed by #351
Closed

Proposed change to the __repr__ method of the Derivatives class #348

scott-huberty opened this issue Feb 28, 2024 · 1 comment · Fixed by #351
Labels
enhancement New feature or request

Comments

@scott-huberty
Copy link
Contributor

scott-huberty commented Feb 28, 2024

Hello,

I was peeking around the code again (debugging some issues on my side), and was trying to inspect the Derivatives instance created during my nibabies run.

However, the current __repr__ method only shows attributes (with matching keys in self.names) that have non-None values. This was confusing, because those attributes are always initially set to None, and only get populated once the populate method is called.. When everything is None, it appears as if the entire Derivatives instance is None:

def __repr__(self):
return '\n'.join([name for name in self.names if getattr(self, name)])

Current behavior (toy example):

Code
class DerivativesMain:
    def __init__(self, bids_root):
        self.names = dict(t1w_mask=dict(), t2w_mask=dict(), t1w_aseg=dict(), t2w_aseg=dict())
        for name in self.names:
            setattr(self, name, None)
    def __repr__(self):
         return '\n'.join([name for name in self.names  if getattr(self, name)])
foo = DerivativesMain("foo")
foo
>

Proposed change

Always include information about attributes with matching keys in self.names, regardless of their current value, in the repr string:

Code
class DerivativesPR:
    def __init__(self, bids_root):
        self.names = dict(t1w_mask=dict(), t2w_mask=dict(), t1w_aseg=dict(), t2w_aseg=dict())
        for name in self.names:
            setattr(self, name, None)

    def __repr__(self):
        return '\n'.join(f"{attr}: {getattr(self, attr)}" for attr in self.names)
bar = DerivativesPR("bar")
bar
t1w_mask: None
t2w_mask: None
t1w_aseg: None
t2w_aseg: None

This representation would make it easier to verify the values for the expected attributes, helping in debugging (and in my case, I would have immediately known that I had done something wrong on my end 🙂 )

If this sounds reasonable to you, I can start a PR.

@mgxd
Copy link
Collaborator

mgxd commented Feb 28, 2024

Yup, I agree it makes sense to display all names even before populating the class - feel free to open a PR!

@mgxd mgxd added the enhancement New feature or request label Feb 28, 2024
@mgxd mgxd closed this as completed in #351 Mar 27, 2024
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 a pull request may close this issue.

2 participants