Skip to content

Conversation

@tylerflex
Copy link
Collaborator

@tylerflex tylerflex commented Sep 9, 2022

Both hdf5 and json should have it now.

@tylerflex tylerflex force-pushed the feature/type_in_dataarray branch from 6a8d8d0 to 31b701c Compare September 9, 2022 09:24
@tylerflex tylerflex force-pushed the feature/type_in_dataarray branch from 5b52b27 to 678982f Compare September 9, 2022 10:16
@momchil-flex
Copy link
Collaborator

This does not add the type tag to data arrays in SimulationData. It seems to me that the way it is set up now, it only adds it if you do something like simulation.to_file("simulation.json", data_file="extra_data.hdf5"), but not if you do e.g. sim_data.to_file("sim_data.hdf5"). I think the type tag handling needs to be moved to _save_group_data ?

@tylerflex
Copy link
Collaborator Author

ok, maybe I'm a bit unsure what we need then. so we want to add a type field to DataArray? like in Tidy3dBaseModel? (not just in the file?). Feel free to use this PR as a starting point if you have a clearer idea of what is required for the denormalizer.

@momchil-flex
Copy link
Collaborator

Yeah basically before the data reorg (before 1.6) the data arrays were also Tidy3dBaseModel and had the type field written in the simulation data hdf5. An was using this to define how the different data should be denormalized.

At the very minimum, we need to bring this back in the output of sim_data.to_file("simulation_data.hdf5"). It seem to me that the most straightforward way is if indeed data arrays just had a type tag like base models. However, I don't understand all the ins and outs of that (it seems like sometimes the lack of a type key in the dictionary may be used to differentiate base model from everything else, including a data array?), which is why I'd rather leave it to you or at least hear your input. Technically An can also work without this, but then he has to define in the denormalizer what types of DataArrays each of the MonitorData elements contain, which is not ideal (requires manual change if we change things on our side).

@tylerflex
Copy link
Collaborator Author

tylerflex commented Sep 12, 2022

I just pushed a commit that adds type to the hdf5 by adding it to save_group_data, as well as some tests. it did seem my tests missed this case before. So it should be ok now. Note that the types should be both in hdf5 and json and if he is working with the SimulationData directly, the type is can be accessed through data_array.__class__.__name__. If it makes it easier, he can add a

@property
def type(self):
    return self.__class__.__name__

to DataArray but I think for now it's not needed and I'm slightly worried about it messing up any internals with the super class xr.DataArray.

@momchil-flex
Copy link
Collaborator

The denormalizer actually works with the hdf5 file directly without using tidy3d frontend at all, currently.

@momchil-flex momchil-flex merged commit 6398a31 into develop Sep 12, 2022
@tylerflex tylerflex deleted the feature/type_in_dataarray branch September 13, 2022 06:28
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.

3 participants