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

Improve potential bottlenecks in compute_Sv #1200

Closed
lsetiawan opened this issue Oct 26, 2023 · 7 comments · Fixed by #1285
Closed

Improve potential bottlenecks in compute_Sv #1200

lsetiawan opened this issue Oct 26, 2023 · 7 comments · Fixed by #1285

Comments

@lsetiawan
Copy link
Member

Below are the 2 potential places where the calibration code (compute_Sv) code can have bottleneck:

  • in get_vend_cal_params_power where a broadcasting is probably used to get a data variable into the "right" shape based on indexing:

    # Get param dataarray into correct shape
    da_param = (
    vend[param]
    .expand_dims(dim={"ping_time": idxmin["ping_time"]}) # expand dims for direct indexing
    .sortby(idxmin.channel) # sortby in case channel sequence differs in vend and beam
    )

  • in harmonize_env_param_time where there is either a check for all the timestamps or an interpolation:

    # If there's only 1 time1 value,
    # or if after dropping NaN there's only 1 time1 value
    if p["time1"].size == 1 or p.dropna(dim="time1").size == 1:
    return p.dropna(dim="time1").squeeze(dim="time1").drop("time1")
    # Direct assignment if all timestamps are identical (EK60 data)
    elif np.all(p["time1"].values == ping_time.values):
    return p.rename({"time1": "ping_time"})
    elif ping_time is None:
    raise ValueError(f"ping_time needs to be provided for interpolating {p.name}")
    else:
    return p.dropna(dim="time1").interp(time1=ping_time)

Originally posted by @leewujung in #1165 (comment)

@anantmittal
Copy link
Contributor

pytest -vvrP echopype/tests/calibrate/test_cal_params.py::test_get_vend_cal_params_power
pytest -vvrP echopype/tests/calibrate/test_env_params.py::test_harmonize_env_param_time

@anantmittal
Copy link
Contributor

@leewujung Could you explain a bit more about the potential bottleneck in get_vend_cal_params_power ? Do we want to parallelize specific operations using dask? Or do we want to refactor the code using numpy/xarray itself to improve runtime?

@anantmittal
Copy link
Contributor

@leewujung Similar question for improvement in harmonize_env_param_time . Are we looking into parallelizing specific (e.g., np.all) operations?

@leewujung
Copy link
Member

Do we want to parallelize specific operations using dask? Or do we want to refactor the code using numpy/xarray itself to improve runtime?

I think it is both. We want to enable handling data with lazy-loaded arrays if it's not already, and ensure that the operations are distributed efficiently when dask supports them (and if not, see where dask is heading on the ops, since these ops are not very specific).

In get_vend_cal_params_power, I worry that expand_dims and to a lesser degree sortby is expensive.

In harmonize_env_param_time, the np.all in one of the conditions may be slow if time dimension is large, and .interp may also be slow.

@anantmittal
Copy link
Contributor

For harmonize_env_param_time ,

On running some experiments, np.all appears to be the fastest way when using just numpy. This link also shares the same thought.

@leewujung
Copy link
Member

This harmonize_env_param_time component was addressed in #1235.

@leewujung
Copy link
Member

We can close this now with #1235 and #1285 merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants