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

__repr__ and __str__ for models #652

Merged
merged 8 commits into from Jul 3, 2019
Merged

__repr__ and __str__ for models #652

merged 8 commits into from Jul 3, 2019

Conversation

pmli
Copy link
Member

@pmli pmli commented Mar 22, 2019

This includes some basic __repr__ and __str__ methods for models. Comments are welcome.

Closes #392.

@pmli pmli added pr:new-feature interfaces labels Mar 22, 2019
@pmli pmli added this to the 2019.2 milestone Mar 22, 2019
@pmli pmli requested a review from sdrave Mar 22, 2019
@sdrave
Copy link
Member

@sdrave sdrave commented Apr 5, 2019

I see that you have decided to let __str__ and __repr__ behave quite differently. Actually, I am not really sure what we want here. I know that it is recommended practice to let the result of __repr__ be mostly equivalent to the Python code needed to instantiate the object while __str__ should give a nice string representation. However, our objects might be a little too complex for such a __repr__ implementation. In particular, if proper __repr__ functions are implemented for operators as well. (Think of a parameterized problem, where multiple operators are (probably nested) LincombOperators.) Also, I am not sure if we really want to print all (optional) arguments of the construtor call.

@pymor/pymor-devs , what do you think?

@pmli
Copy link
Member Author

@pmli pmli commented Apr 5, 2019

Ok, I'm actually fine with using __repr__ for "pretty printing" to avoid using print. I wasn't sure how much we want (or how much it makes sense) to follow the conventions (__repr__, __str__).

@sdrave
Copy link
Member

@sdrave sdrave commented Apr 16, 2019

This is really something, where I am not sure what the right thing is. Maybe someone else also has an opinion, @pymor/pymor-devs?

@renefritze
Copy link
Member

@renefritze renefritze commented Apr 17, 2019

I also think it's fine to break from the __repr__ rule for those kinds of complex objects.

@pmli
Copy link
Member Author

@pmli pmli commented Apr 17, 2019

@pymor/pymor-devs How about first adding __str__ in this PR and later discuss __repr__? I would like to hear what do you think print(model) should look like for StationaryModel and InstationaryModel (and iosys models if you have ideas).

@sdrave
Copy link
Member

@sdrave sdrave commented May 21, 2019

I guess for StationaryModel i would like something like

f'''
StationaryModel {self.name if self.name != 'StationaryModel' else ''}
    parameter_space: {self.parameter_space}
    solution_space: {self.solution_space}
    operator: {self.operator}
    rhs: {self.rhs}
'''

If the str of self.operator or self.rhs has multiple lines, it should be indented accordingly.

src/pymor/models/basic.py Outdated Show resolved Hide resolved
src/pymor/models/basic.py Outdated Show resolved Hide resolved
src/pymor/models/basic.py Outdated Show resolved Hide resolved
src/pymor/models/basic.py Show resolved Hide resolved
src/pymor/models/iosys.py Outdated Show resolved Hide resolved
@sdrave sdrave mentioned this pull request May 29, 2019
@pmli
Copy link
Member Author

@pmli pmli commented May 31, 2019

How about separating this PR into one about __repr__ and one about __str__?

@sdrave
Copy link
Member

@sdrave sdrave commented Jun 19, 2019

With #706 merged, how do we want to proceed here? I think it is clear that we won't need a custom __repr__ for Models. What about __str__? I guess I would be fine with just using the __repr__ for now. Does anybody think we need something prettier? If yes, do we want recursion in __str__ similar to what is done in __repr__? (I would say that if we want a custom __str__, then we should actually avoid recursion and go for a short representation.)

@pmli
Copy link
Member Author

@pmli pmli commented Jun 19, 2019

I would like to have a prettier __str__ for system classes, where it's easier to see the number of inputs, output, and states (and some other useful information).

@sdrave
Copy link
Member

@sdrave sdrave commented Jun 19, 2019

Ok, then I would propose to change __str__ of StationaryModel and InstationaryModel to not print the contained operators.

Can you rebase on master? Then I would make the changes to these methods.

@sdrave
Copy link
Member

@sdrave sdrave commented Jul 1, 2019

@pmli, made the changes. Please have a look.

- prints parameter space instead of type
- some reordering
@pmli
Copy link
Member Author

@pmli pmli commented Jul 1, 2019

@sdrave I've copied the parameter_space and solution_space parts to iosys models and did some reordering. Let me know what you think.

@sdrave
Copy link
Member

@sdrave sdrave commented Jul 3, 2019

LGTM.

@pmli pmli merged commit 6fa6e21 into master Jul 3, 2019
7 checks passed
@pmli pmli deleted the model-str-repr branch Jul 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:new-feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants