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

Adapt ACPWorkflow to embedded cdb file #395

Merged
merged 4 commits into from
Feb 6, 2024

Conversation

janvonrickenbach
Copy link
Contributor

CDB file is now embedded in the acph5 file, so we build the workflow either from the cdb or the acph5 file. This currently does not support the use case of opening pre-existing acph5 files which don't yet have the cdb embedded. I would consider this a niche use-case since users currently don't typically work in standalone. If they do they can still just re-save the ACP model.

h5_file_path:
The path to the h5 file.
acph5_file_path:
The path to the acph5 file.
"""

def __init__(
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if the __init__ in this form still makes sense: its body is mostly just distinguishing between the different input file options.
Are we still expecting users to call this manually? Otherwise, we might as well directly use parameters local_file_path and format.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I would expect users to only use the classmethod. I will implement your suggestion.

Copy link
Member

@greschd greschd Feb 6, 2024

Choose a reason for hiding this comment

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

Ok, it should probably also be mentioned in the class docstring to use the classmethods for construction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is mentioned in the class docstring. Do you mean somewhere else?

Copy link
Member

Choose a reason for hiding this comment

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

No, sorry I hadn't actually looked at the docstring 😅 you were ahead of me here.

h5_file_path:
The path to the h5 file.
acph5_file_path:
The path to the acph5 file.
"""

def __init__(
Copy link
Member

Choose a reason for hiding this comment

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

No, sorry I hadn't actually looked at the docstring 😅 you were ahead of me here.

@janvonrickenbach janvonrickenbach enabled auto-merge (squash) February 6, 2024 15:28
@janvonrickenbach janvonrickenbach merged commit 9b2a437 into main Feb 6, 2024
12 checks passed
@janvonrickenbach janvonrickenbach deleted the feat/use_embedded_cdb_file branch February 6, 2024 15:46
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