-
Notifications
You must be signed in to change notification settings - Fork 168
Adding reprs for all relevant classes in v4 #2452
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
base: v4-dev
Are you sure you want to change the base?
Conversation
As it is not used anymore in v4
And also adding support for dictionaries in the _format_list_items_multiline function
And also updating one repr test
|
I think that some of these reprs for the complex objects are too verbose. e.g., for a fieldset, I can imagine a user when printing an object only really cares about the names of the available fields, vectorfields, constants, and values for constants - so that they can write their kernels. I don't think they care about the underlying data for the fields (and they can poke at that if they want anyway). Especially in v4 where they should be checking their data before they pass to Parcels. I think having a repr that is too verbose is a downside since it floods the output meaning they have to pick through for items of interest. I think there's also an argument to be made that maybe we should be teaching users via the docs that they should just use Deciding what's important to users (new and migrating from v3), and hence what we want to communicate to them via reprs or via other ways is difficult to do without having alpha release testing - I don't think we can make an informed decision now. I'm erring on the side of postponing this From a dev POV, do you feel like any of the reprs in |
| def variable_repr(var: Any) -> str: | ||
| """Return a pretty repr for Variable""" | ||
| return f"Variable(name={var._name!r}, dtype={var.dtype!r}, initial={var.initial!r}, to_write={var.to_write!r}, attrs={var.attrs!r})" | ||
|
|
||
|
|
||
| def timeinterval_repr(ti: Any) -> str: | ||
| """Return a pretty repr for TimeInterval""" | ||
| return f"TimeInterval(left={ti.left!r}, right={ti.right!r})" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we move these back? For short bodies, or oneliners like these which don't use any formatting helper functions I think its better to keep them with the rest of the class body rather than in a separate file
|
Saying all that - once #2452 (comment) is resolved I'm also open to merging. These are very orthogonal changes and we can iterate on gradually :) |
This PR implements #2392 by cleaning up the
__repr__for the most important Parcels classes. It now includesTo be done
See below the result of these
__reprs__for the example of thetutorial_output.ipynb(after thepset.execute())