-
Notifications
You must be signed in to change notification settings - Fork 280
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
doc: add minimal structure to SkeletonDataset class #2991
Conversation
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.
Nice!! Looking forward to review, but this looks good so far.
Actually I was expecting to do more with this as I went along implementing my new frontend (PR still not open), but I didn't need to. This is now ready for review. |
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.
One minor comment but everything looks good to me.
@@ -143,7 +148,8 @@ def _parse_parameter_file(self): | |||
# self.geometry <= a lower case string | |||
# ("cartesian", "polar", "cylindrical"...) | |||
# (defaults to 'cartesian') | |||
pass | |||
self.current_time = -1 # required, change this ! |
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.
It might be worth suggesting what a good value would be for this in a code that doesn't have time dependence. Since it is required, a helpful suggestion might be good here.
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.
Good point. This didn't occur to me. Actually I don't know why it's required at all. I should look for the reason first.
Do you think ˋ0.0` would make sense here ?
This seems fine to me. My suggestion would be a time of zero for default, but setting to -1 makes it so that they explicitly have to set it if they don't want some weird value. |
So I'm down with whatever default value you deem most sensible @munkm , though I don't know what would be a helpful suggestion here :/ |
I think leaving it as -1 in the code but having an inline comment that we suggest 0 would work. |
done ! |
PR Summary
edit:
the changes to
SkeletonDataset
make it so the following code pass without errorwhere the only changes required from the author of a new frontend are
yt/frontends/api.py
NewDataset._is_valid()