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

Fix updater for hdf5 files with custom data, and add hdf5 files to automatic updater testing #1200

Merged
merged 1 commit into from
Oct 11, 2023

Conversation

momchil-flex
Copy link
Collaborator

No description provided.

@momchil-flex momchil-flex changed the base branch from pre/2.5 to develop October 10, 2023 23:45
Copy link
Collaborator

@tylerflex tylerflex left a comment

Choose a reason for hiding this comment

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

just a couple minor comments but looks good otherwise, thanks

@@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Fixed
- Properly handle `.freqs` in `output_monitors` of adjoint plugin.
- Simulation updater for `.hdf5` files with custom data.
Copy link
Collaborator

Choose a reason for hiding this comment

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

custom source more specifically? I thought the existing one seemed to work with custom medium?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I only tried custom source and custom medium and yeah for some reason custom medium seemed fine. However I haven't tried other custom data, so not sure. Also not sure if there's a way to create the custom medium in which it will error (e.g. passing an xarray object as opposed to one of our inherited ones?) So maybe safer to leave as is?

Copy link
Collaborator

Choose a reason for hiding this comment

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

sure sounds good

@@ -187,7 +192,10 @@ def test_validation_speed(tmp_path):
@pytest.mark.parametrize("sim_file", SIM_FILES)
def test_simulation_updater(sim_file):
"""Test that all simulations in ``SIM_DIR`` can be updated to current version and loaded."""
sim_updated = td.Simulation.from_file(sim_file)
if sim_file[-2:] == "h5":
Copy link
Collaborator

Choose a reason for hiding this comment

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

we could also consider just changing from_file to handle .h5 by adding it here:
https://github.com/flexcompute/tidy3d/blob/develop/tidy3d/components/base.py#L223-L228

probably better while we're at it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated, have a quick look?

@momchil-flex momchil-flex merged commit af7552e into develop Oct 11, 2023
16 checks passed
@momchil-flex momchil-flex deleted the momchil/hdf5_updater branch October 11, 2023 22:05
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

2 participants