Separate simulation metadata from (sxs) metadata #64
Conversation
| @@ -69,6 +69,8 @@ def get(self, sim_name): | |||
| metadata = self.get_metadata(sim_name) | |||
| if type(metadata) is not dict and hasattr(metadata, "to_dict"): | |||
| metadata = metadata.to_dict() | |||
| elif isinstance(metadata, dict): | |||
| metadata = dict(metadata.items()) | |||
There was a problem hiding this comment.
I am confused why this check is needed. Wouldn't metadata before and after this action be the same thing?
There was a problem hiding this comment.
This is just a quick fix for another small issue. Tbh, I think it could be addressed more appropriately from the sxs side.
The issue is that somehow the type of metadata becomes the sxs.Metadata, and the update method of which supports syntax like: metadata.update(new_meta), but not metadata.update(**new_data).
But then the latter syntax is used in here. Since I am not sure whether sxs expects the metadata there has to be a pure dict or that they are not aware of this issue, I opt for this simple fix here.
Going back to the original question, the sxs.Metadata is a subclass from OrderedDict, so the isinstance and items() methods work, and converting it to a simple dict revert the update method back to the usual one.
nrcatalogtools/waveform.py
Outdated
| @@ -165,7 +165,7 @@ def load_from_h5(cls, file_path_or_open_file, metadata={}, verbosity=0): | |||
| data[:, idx] = amp_interp(times) * np.exp(1j * phase_interp(times)) | |||
|
|
|||
| w_attributes = {} | |||
| w_attributes["metadata"] = metadata | |||
| # w_attributes["metadata"] = metadata | |||
There was a problem hiding this comment.
No need to comment this line out, please remove it entirely. Please also remove the other commented out # w_attribute below.
Sure, let me have a look at that! But since |
|
Actually, may I ask why these two attributes are set as class attributes? (see here) # Set the file path attribute
cls._filepath = h5_file.filename
# If _metadata is not already
# a set attribute, then set
# it here.
try:
cls._sim_metadata
except AttributeError:
cls._sim_metadata = metadataTo me, it seems to make more sense to set them as instance attributes later in the The current behaviour is like this: from nrcatalogtools import RITCatalog
rit_catalog = RITCatalog.load(download=False)
rit_code = 'RIT:BBH:0001-n100-id3'
h_rit_1 = rit_catalog.get(rit_code)
print('BBH:0001:', h_rit_1.sim_metadata['catalog_tag'])
print('BBH:0001:', h_rit_1._filepath)
rit_code = 'RIT:BBH:0003-n100-id0'
h_rit_3 = rit_catalog.get(rit_code)
print('BBH:0003:', h_rit_3.sim_metadata['catalog_tag'])
print('BBH:0003:', h_rit_3._filepath)
## Output:
# BBH:0001: RIT:BBH:0001
# BBH:0001: None
# BBH:0003: RIT:BBH:0001
# BBH:0003: None(I can quickly push a commit that fixes this) |
This pull request aims to fix #63.
The current behaviour of the metadata in
nrcatalogtools.waveform.WaveformModesis that after the instance is created, the simulation metadata is stored in bothself.metadataandself.sim_metadata, whereas the necessary metadata needed for the internals ofsxs(e.g.TimeSeriesandWaveformModes) to work is stored inself._metadata.This is the current behaviour explicitly:
This mismatch between
metadataand_metadatacauses issues in operations as asxs.WaveformModesorsxs.TimeSeries. In particular, whenever thendarray.view()method is invoked (e.g. [1] and [2]), the keys in_metadatawill be overwritten/combined with those inmetadata, which then lead to the slicing issue in #63.This PR addresses this issue by creating an additional hidden attribute
_sim_metadatato store the simulation metadata, thereby completely separatemetadatafromsim_metadata.After this PR, the behaviour should become: