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

[TEP002] Add a to_hdf method to the Radial1DModel #589

Merged
merged 2 commits into from
Jun 24, 2016

Conversation

ftsamis
Copy link
Member

@ftsamis ftsamis commented Jun 14, 2016

Added a model.to_hdf method which in turn calls plasma.to_hdf and also stores t_inner, ws, t_rads, v_inner, v_outer.

Additionally, atom_data_uuid is also stored in a Series under the HDF structure path model/metadata.


self.plasma.to_hdf(hdf_store, os.path.join(path, 'model'),
False, plasma_properties)
try:
Copy link
Member

Choose a reason for hiding this comment

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

remove this - runner will be stored as part of simulation

output_value = output_value.si.value
if np.isscalar(output_value):
output_value = [output_value]
data = pd.DataFrame(output_value)
Copy link
Contributor

Choose a reason for hiding this comment

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

May I suggest creating one Dataframe (called properties or smth) and then adding the numpy arrays as columns?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I hadn't noticed that they are all the same length. Is that always the case? Because otherwise this won't work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are they exactly the same length? I don't know but it's not important. If
one element is longer than the rest the others get filled with np.nan. One
would have to take care of that when reading the valus, that's all

Fotis Tsamis notifications@github.com schrieb am Mo., 20. Juni 2016 um
17:18 Uhr:

In tardis/model.py
#589 (comment):

@@ -268,6 +268,16 @@ def to_hdf(self, path_or_buf, path='', plasma_properties=None):
None

     """
  •    properties = ['t_inner', 'ws', 't_rads', 'v_inner', 'v_outer']
    
  •    for p in properties:
    
  •        output_value = getattr(self, p)
    
  •        if hasattr(output_value, 'si'):
    
  •            output_value = output_value.si.value
    
  •        if np.isscalar(output_value):
    
  •            output_value = [output_value]
    
  •        data = pd.DataFrame(output_value)
    

Yeah, I hadn't noticed that they are all the same length. Is that always
the case? Because otherwise this won't work.


You are receiving this because you commented.

Reply to this email directly, view it on GitHub
https://github.com/tardis-sn/tardis/pull/589/files/b9ca672ee6796a356ca4593f1e8886adfb82397a..d8f3a53a09090c6946f8f366ae5e376fb6e68b4c#r67708417,
or mute the thread
https://github.com/notifications/unsubscribe/AF4OjmRNXUTretIFxgKxUjopvBLYHhu0ks5qNq9DgaJpZM4I1pWa
.

Copy link
Member Author

Choose a reason for hiding this comment

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

As far as I checked it does not do that automatically.

>>>pd.DataFrame({'a': [1,2], 'b' : [1,2,3]})
ValueError: arrays must all be same length

while:

>>> pd.DataFrame({'a': [1,2], 'b' : [1,2]})
   a  b
0  1  1
1  2  2

So one would have to take care of manually adding np.nan for the missing values of shorter columns as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

You want to do it this way:

df = pd.DataFrame()
df['a'] = [1,2,3]
df['b'] = [1,2]

Fotis Tsamis notifications@github.com schrieb am Mo., 20. Juni 2016 um
18:56 Uhr:

In tardis/model.py
#589 (comment):

@@ -268,6 +268,16 @@ def to_hdf(self, path_or_buf, path='', plasma_properties=None):
None

     """
  •    properties = ['t_inner', 'ws', 't_rads', 'v_inner', 'v_outer']
    
  •    for p in properties:
    
  •        output_value = getattr(self, p)
    
  •        if hasattr(output_value, 'si'):
    
  •            output_value = output_value.si.value
    
  •        if np.isscalar(output_value):
    
  •            output_value = [output_value]
    
  •        data = pd.DataFrame(output_value)
    

As far as I checked it does not do that automatically.

pd.DataFrame({'a': [1,2], 'b' : [1,2,3]})ValueError: arrays must all be same length

while:

pd.DataFrame({'a': [1,2], 'b' : [1,2]})
a b0 1 11 2 2

So one would have to take care of manually adding np.nan for the missing
values of shorter columns as well.


You are receiving this because you commented.

Reply to this email directly, view it on GitHub
https://github.com/tardis-sn/tardis/pull/589/files/b9ca672ee6796a356ca4593f1e8886adfb82397a..d8f3a53a09090c6946f8f366ae5e376fb6e68b4c#r67725293,
or mute the thread
https://github.com/notifications/unsubscribe/AF4OjnmBdZ7eP1VmVYuyElrjD-I0Rjd_ks5qNsZEgaJpZM4I1pWa
.

Copy link
Member Author

Choose a reason for hiding this comment

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

That was unexpected to me. Alright, thanks for pointing this out.

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you sure this works? Because the exact snippet you gave throws a ValueError: Length of values does not match length of index for me.

Copy link
Member

Choose a reason for hiding this comment

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

I think there is a high likelihood that this doesn't work. But all of the values in model have the same length.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was actually trying to do the same with the plasma properties, so we could have a uniform way of storing things.

@wkerzendorf
Copy link
Member

@tardis-sn/tardis-core can someone else look at this - from my point of view it looks mergeable.

Store the v_inner, v_outer as model attributes. Furthermore, convert v_middle from a property to an attribute as well
@ftsamis
Copy link
Member Author

ftsamis commented Jun 23, 2016

Note to self: After this is merged, I should rebase #601 on to_hdf and change the plasma.to_hdf call which is made in the model.to_hdf to match the new signature.

@wkerzendorf
Copy link
Member

@tardis-sn/tardis-core looks good to me - should we merge?

@ftsamis
Copy link
Member Author

ftsamis commented Jun 23, 2016

I think we should merge this, since either way it's going to face some minor changes because of some additions to the way data are stored to HDF in #601. Then, when both this and #601 are merged we can discuss if we are happy to merge the entire to_hdf branch to master. For now, this also blocks #601 from having its couple last commits.

@wkerzendorf wkerzendorf merged commit f402cd1 into tardis-sn:to_hdf Jun 24, 2016
@ftsamis ftsamis deleted the model-to_hdf branch December 20, 2016 15:32
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.

None yet

3 participants