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

.load vs .load_results #27

Open
ronald-jaepel opened this issue Nov 20, 2024 · 3 comments
Open

.load vs .load_results #27

ronald-jaepel opened this issue Nov 20, 2024 · 3 comments
Milestone

Comments

@ronald-jaepel
Copy link
Collaborator

ronald-jaepel commented Nov 20, 2024

Was it on purpose that .load does not work with the DLL interface and only .load_results actually loads the results?

Looking more into it, it might be that .load is from the HDF5 interface. We should consider re-routing it to load_results.

@ronald-jaepel
Copy link
Collaborator Author

ronald-jaepel commented Nov 20, 2024

After some thought, I think

There should be one-- and preferably only one --obvious way to do it.

so we should remove/rename one of them. My vote is for:

rename .load_results() to .load(). It loads the results, that's what was wide-spread in use in the previous interface in my perception.
rename .load() to ._load() or .load_from_h5() or something.

Especially because right now sim.run_load() and sim.run() & sim.load() do not do the same thing, which I think is confusing.

For reference, here's the function in question as they are now:

# in class H5 interface
def load(self, paths: Optional[List[str]] = None, update: bool = False, lock: bool = False) -> None:
    """
    Load data from the specified HDF5 file.

    Parameters
    ----------
    paths : Optional[List[str]], optional
        Specific paths to load within the HDF5 file.
    update : bool, optional
        If True, updates the existing data with the loaded data.
    lock : bool, optional
        If True, uses a file lock while loading.
    """

# in class Cadet:
def load_results(self) -> None:
    """Load the results of the last simulation run into the current instance."""
    runner = self.cadet_runner
    if runner is not None:
        runner.load_results(self)
    else:
        raise RuntimeError("No CADET Runner found.")

If you agree @schmoelder I can create a PR for that.

@schmoelder
Copy link
Contributor

Sure, sounds good.

@schmoelder schmoelder reopened this Dec 10, 2024
@schmoelder schmoelder added this to the v1.1.0 milestone Dec 11, 2024
@schmoelder
Copy link
Contributor

schmoelder commented Dec 11, 2024

Proposal

New Interface:

Cadet.run_simulation() -> Run simulation, proxy to Runner.run() + Runner.load_results()
Cadet.load_from_file() -> Load full simulation, proxy to H5.load_from_file() - new correct way

Cadet.load_results() -> Load results, proxy to Runner.load_results() - with deprecation warning -> use.load_from_file()
Cadet.run_load() -> Call runner.run(), runner.load_results() - with deprecation warning -> use .run_simulation()
Cadet.load() -> Load full simulation, proxy to H5.load_from_file() - with deprecation warning -> use.load_from_file()
Cadet.run() -> Proxy to Runner.run() - with deprecation warning -> use Cadet.run_simulation()

Runner.run() -> Run simulation using implementation by runner
Cadet_DLL_runner.run()
Cadet_CLI_runner.run()

Runner.load_results() -> Load only results using implementation by runner
Cadet_DLL_runner.load_results()
Cadet_CLI_runner.load_results() (using H5.load_from_file(["output" / "meta"]))

H5.load_from_file() -> Load results from H5 file (optionally, load only specific branch)

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

No branches or pull requests

2 participants