-
Notifications
You must be signed in to change notification settings - Fork 40
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
Removing indent in string stored in hdf5 #1228
Conversation
b2ba1cb
to
413f0c9
Compare
tidy3d/components/base.py
Outdated
@@ -563,7 +563,7 @@ def to_hdf5(self, fname: str, custom_encoders: List[Callable] = None) -> None: | |||
|
|||
with h5py.File(fname, "w") as f_handle: | |||
|
|||
f_handle[JSON_TAG] = self._json_string | |||
f_handle[JSON_TAG] = self._json() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this indent=None
by default? maybe it's more clear to pass it explicitly?
ab81cbe
to
6de60b9
Compare
I've updated this PR to use tidy3d/tidy3d/web/core/task_core.py Lines 29 to 34 in 07a27f2
I think the logic is that I shouldn't be importing anything from tidy3d/components here, so I can't use something like
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
string splitting Looks good, a few suggestions for making it a bit simpler.
tidy3d/components/base.py
Outdated
f_handle[JSON_TAG] = self._json_string | ||
json_str = self._json_string | ||
for ind in range(ceil(len(json_str) / MAX_STRING_LENGTH)): | ||
key = f"{JSON_TAG}_{ind}" if ind > 0 else JSON_TAG |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can write a static method or module level function for this to avoid duplicating the key as a function of index, incase we change this or need to use it elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in Tidy3DBaseModel
@staticmethod
def _json_string_key(index):
if index:
return f"{JSON_TAG}_{ind}"
return JSON_TAG
to_hdf5
json_str = self._json_string
num_strings = ceil(len(json_str) / MAX_STRING_LENGTH)
for ind in range(num_strings):
ind_start = int(ind * MAX_STRING_LENGTH)
ind_stop = min(int(ind + 1) * MAX_STRING_LENGTH, len(json_str))
key = self. _json_string_key(ind)
f_handle[key] = json_str[ind_start:ind_stop]
@classmethod
def _json_string_from_hdf5(cls, fname: str) -> str:
"""Load the model json string from an hdf5 file."""
with h5py.File(fname, "r") as f_handle:
num_string_parts = len([key for key in f_handle.keys() if JSON_TAG in key])
json_string = ""
for ind in range(num_string_parts):
key = _json_string_key(ind)
json_string += f_handle[key][()]
return json_string
tidy3d/components/base.py
Outdated
f_handle[JSON_TAG] = self._json_string | ||
json_str = self._json_string | ||
for ind in range(ceil(len(json_str) / MAX_STRING_LENGTH)): | ||
key = f"{JSON_TAG}_{ind}" if ind > 0 else JSON_TAG |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this creates JSON_STRING, JSON_STRING_1, ...
?
I'm curious if it will make the logic simpler to just not perform any special handling for ind=0
, like JSON_STRING_0, JSON_STRING_1, ...
. If the string is short enough to fit in JSON_STRING_0
, we just leave it named like that. Will it break backwards compatibility with script loading maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
anyway, see comment and suggestion above, I think it makes things a bit simpler to keep the handling how it is but just have a single function to get the key, treating ind=0 differently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, yeah not appending ind=0
is for backwards compatibility.
Regarding |
So far, the way we have been dealing with importing tidy3d things into web api is through passing those things during initialization of web api here Line 32 in 07a27f2
For example, currently we pass log , logging console (should change that to logging console getter eventually), and tidy3d version. So, perhaps _json_string_from_hdf5() could also be set in web api from tidy3d in the same way? That is:
|
yea perhaps this is a way to do it, although it seems a bit strange potentially. Maybe an alternative approach is to handle this in the SimulationTask, grab the class of the component for the task and then call |
Yeah, I think it would work there. However, the other place where json string is extracted from hdf5 is here tidy3d/tidy3d/web/api/tidy3d_stub.py Lines 152 to 169 in 07a27f2
|
I see, that's a bit messy, maybe the easiest approach is to just duplicate but make a note and get qingeng to help unify things, but passing in the config also works (even though it seems like not the long term solution to me). |
6de60b9
to
712f140
Compare
Updated, with code duplicated in |
712f140
to
e375724
Compare
# default indentation (# spaces) in files | ||
INDENT = 4 | ||
|
||
INDENT_JSON_FILE = 4 # default indentation of json string in json files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could consider setting this to 2 to reduce the size of the file itself, but no preference either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: I'm not sure this will actually change the file size. depends on how they json handles indents I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested. It does change the size. I guess let's do 2 yeah? But this shouldn't matter for upload/download from the server because all this is done through hdf5 files now. So not as important potentially, could be left 4 since it's prettier?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fine by me. I prefer 4 if the file size is not an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, thank you! just a couple minor notes
tidy3d/components/base.py
Outdated
@@ -430,6 +434,23 @@ def get_sub_model(cls, group_path: str, model_dict: dict | list) -> dict: | |||
model_dict = model_dict[key] | |||
return model_dict | |||
|
|||
@staticmethod | |||
def _json_string_key(index): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe add some type annotations just while we're at this
def _json_string_key(index: int) -> str:
e375724
to
f58e161
Compare
No description provided.