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

Adds __repr__ for our custom classes #286

Merged
merged 38 commits into from
Jan 15, 2025
Merged

Adds __repr__ for our custom classes #286

merged 38 commits into from
Jan 15, 2025

Conversation

billbrod
Copy link
Member

@billbrod billbrod commented Jan 9, 2025

This adds a __repr__ method for our custom classes: basis, glm, observation models, and regularizers. The logic for almost all of these are included in the new nmo.utils.format_repr function, summarized as:

- The returned string includes the output of ``obj.get_params(deep=False)`` if value
  (that is, if it's not False, None, empty, etc.).

- Any key in ``exclude_keys`` are excluded.

- If the key is in ``use_name_keys``, we use ``value.__name__``. Else we use
  ``value.__repr__``

TransformerBasis and CompositeBasis are different:

  • TransformerBasis is Transformer(self.basis)
  • CompositeBasis is
AdditiveBasis(
    basis1=self.basis1,
    basis2=self.basis2,
)

and same for Multiplicative, with composites of composites using additional tab indenting

Copy link
Collaborator

@BalzaniEdoardo BalzaniEdoardo left a comment

Choose a reason for hiding this comment

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

thanks for the first pass at this!

I added a couple of comments. I would include a max depth or something similar for additive bases in the repr.

src/nemos/utils.py Outdated Show resolved Hide resolved
src/nemos/utils.py Outdated Show resolved Hide resolved
src/nemos/utils.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jan 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.40%. Comparing base (a510ef3) to head (67da7b4).
Report is 110 commits behind head on development.

Additional details and impacted files
@@               Coverage Diff               @@
##           development     #286      +/-   ##
===============================================
+ Coverage        96.13%   97.40%   +1.26%     
===============================================
  Files               34       35       +1     
  Lines             2642     2850     +208     
===============================================
+ Hits              2540     2776     +236     
+ Misses             102       74      -28     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@BalzaniEdoardo
Copy link
Collaborator

Added Features

  • Slightly changed the format_repr utility function:
    • Ignore parameters that have the attribute shape (intended for arrays).
    • Do not ignore parameters with value False and 0, while ignore other falsey values like None or "".
  • Added basic repr tests.
  • Doctests for basis.

Note: I did not add doctests for GLM repr because the output is way too long, but I have included actual tests.

Copy link
Member Author

@billbrod billbrod left a comment

Choose a reason for hiding this comment

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

Should probably add a doctest for at least the GLM (if not also regularizer and observation models?)

There are other notes scattered throughout

src/nemos/glm.py Outdated Show resolved Hide resolved
src/nemos/utils.py Outdated Show resolved Hide resolved
src/nemos/utils.py Outdated Show resolved Hide resolved
tests/test_observation_models.py Outdated Show resolved Hide resolved
tests/test_observation_models.py Outdated Show resolved Hide resolved
tests/test_utils.py Show resolved Hide resolved
tests/test_utils.py Outdated Show resolved Hide resolved
tests/test_utils.py Outdated Show resolved Hide resolved
@billbrod
Copy link
Member Author

Most recent change adds label to the repr. This only affects basis right now (others don't have a label param). Questions about that:

  • We don't handle label="" (which user would have to do themselves) in a special way, so you'd get back e.g., (RaisedCosineLinearEval, width=2.0, ...). (With label=None and if the user doesn't set it directly, you'd get RaisedCosineLinearEval(width=2.0, ...))
  • I didn't add the label behavior to any doctests, because I didn't see it being used in any existing doctests. Should I?
  • User can't set label for Additive and Multiplicative basis, so I don't handle it specifically there. But they do have a more informative label -- should I make use of it?
  • With this, I don't think we're using exclude_keys arg anywhere (it's being used in for the GLM, but as I noted in my comments, I think that can be removed) -- should we keep it in case we need it later or remove it?

Copy link
Collaborator

@sjvenditto sjvenditto left a comment

Choose a reason for hiding this comment

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

I've added suggestions for the composite basis repr to have a maximum print length based on the terminal size. The line divisor might be too big (i.e. the print underfills the terminal), but here's an example of a line limited repr in my terminal:
image

src/nemos/basis/_basis_mixin.py Show resolved Hide resolved
src/nemos/basis/_basis_mixin.py Outdated Show resolved Hide resolved
@BalzaniEdoardo BalzaniEdoardo merged commit df793d2 into development Jan 15, 2025
13 checks passed
@BalzaniEdoardo BalzaniEdoardo deleted the repr branch January 15, 2025 17:53
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

Successfully merging this pull request may close these issues.

4 participants