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 local web test and remove simulation.json #584

Merged
merged 1 commit into from
Dec 2, 2022

Conversation

tylerflex
Copy link
Collaborator

No description provided.

@momchil-flex
Copy link
Collaborator

momchil-flex commented Dec 1, 2022

Question I had on slack: what I'm confused about is also why we need the line that explicitly saves the batch in the test? didn't we have something where the batch is saved to file directly when creating?

@tylerflex
Copy link
Collaborator Author

tylerflex commented Dec 2, 2022

ok I think I know what’s going on with the batchdata test.
https://github.com/flexcompute/tidy3d/blob/develop/tests/_test_local/_test_web.py#L258-L263
It’s a bit confusing:

  1. the test has @clear_tmp on it for some reason, so basically the tmp directory is totally wiped before the test is run.
  2. the test then grabs the Batch object and tries to download a file of the batch to tmp. (Note: previously, this was a .json format, but now it only works if .hdf5 , i’ll explain why).
  3. BatchData.load() is then called, which looks for a _batch_path in the tmp directory. https://github.com/flexcompute/tidy3d/blob/develop/tidy3d/web/container.py#L199-L217
  4. Batch._batch_path used to be path + batch.json but now it’s path + batch.hdf5 so this part simply failed before because the test was saving a .json file and looking for a .hdf5 file.

however, we were confused earlier because the test seemed to fail if the batch was not written to file in the test. The reason is because of the first bullet point. Basically clear_tmp wiped the directory so we needed to save some batch file in there before the BatchData can be loaded from path. So:

  • To fix the test:
    • We can either leave the to_file() with .hdf5 format (current commit)
    • or we can simply not clear_tmp after the previous test is done, which I verified also works.
  • We might also want to consider changing how BatchData loads, perhaps by providing a @classmethod to load it from an existing Batch instance? pretty low priority for now i think though.

@momchil-flex momchil-flex merged commit eda9e5c into develop Dec 2, 2022
@tylerflex tylerflex deleted the tyler/batch_fix branch May 16, 2023 14:18
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.

2 participants