-
-
Notifications
You must be signed in to change notification settings - Fork 435
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
[TEP014] PlasmaWriterMixin #752
Conversation
first of all @vg3095 rebase |
tardis/io/util.py
Outdated
data.pop('atomic_data') | ||
return data | ||
|
||
def to_hdf(self, file_path, path='plasma', collection=None): |
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.
this should be the same as the "normal" to_hdf
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.
in this case you should also have class properties by using the property
decorator on the outputs for example.
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.
@wkerzendorf I am facing a small problem here , I don't want name
parameter here for Plasma hdf calls
,
With name
parameter it will save like ex-> /plasma/abundance/abundance
without name parameter /plasma/abundance/
@wkerzendorf Please review. |
@wkerzendorf @vg3095 I think this PR need some rework before we can merge it. Specifically I don't like mixing functionality of the PlasmaHDFWriter into the normal HDFWriter (the changes to the |
1. Add a collection parameter for PlasmaWriterMixin to_hdf method. 2. Use super inside PlasmaWriterMixin to call the original to_hdf method.
@wkerzendorf @yeganer Now, I have updated 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.
looks good.
@tardis-sn/tardis-core can someone else look over this and approve this for merging? |
No description provided.