search

waltsims / k-wave-python

A Python interface to k-Wave GPU accelerated binaries
https://k-wave-python.readthedocs.io/en/latest/
GNU General Public License v3.0
109 stars 32 forks source link

454 element measure precision #457

Closed precicely closed 2 months ago

waltsims commented 3 months ago

It looks like I missed something in my original review. I hope you don't mind that I have merged master into your PR to address conflicts.

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 71.84%. Comparing base (8eca448) to head (fb2dc74). Report is 1 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #457 +/- ## ======================================= Coverage 71.84% 71.84% ======================================= Files 46 46 Lines 6744 6744 Branches 1496 1496 ======================================= Hits 4845 4845 Misses 1333 1333 Partials 566 566 ``` | [Flag](https://app.codecov.io/gh/waltsims/k-wave-python/pull/457/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Walter+Simson) | Coverage Δ | | |---|---|---| | [3.10](https://app.codecov.io/gh/waltsims/k-wave-python/pull/457/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Walter+Simson) | `72.04% <ø> (ø)` | | | [3.11](https://app.codecov.io/gh/waltsims/k-wave-python/pull/457/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Walter+Simson) | `72.04% <ø> (ø)` | | | [3.12](https://app.codecov.io/gh/waltsims/k-wave-python/pull/457/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Walter+Simson) | `72.04% <ø> (ø)` | | | [3.9](https://app.codecov.io/gh/waltsims/k-wave-python/pull/457/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Walter+Simson) | `71.81% <ø> (ø)` | | | [macos-latest](https://app.codecov.io/gh/waltsims/k-wave-python/pull/457/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Walter+Simson) | `71.78% <ø> (ø)` | | | [ubuntu-latest](https://app.codecov.io/gh/waltsims/k-wave-python/pull/457/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Walter+Simson) | `71.81% <ø> (ø)` | | | [windows-latest](https://app.codecov.io/gh/waltsims/k-wave-python/pull/457/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Walter+Simson) | `71.82% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Walter+Simson#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

waltsims commented 3 months ago

Also, in check_element_equality, there is a typo. I missed it in my first review. The argument should be actual_element not actual_elelment. Perhaps something like this would be more readable and efficient as well?

def check_element_equality(
    actual_element: Element,
    expected_element: Element,
) -> bool:
    for field in dataclasses.fields(expected_element):
        expected = getattr(expected_element, field.name)
        actual = getattr(actual_element, field.name)
        if isinstance(expected, np.ndarray):
            if not np.allclose(actual, expected):
                return False
        elif actual != expected:
            return False
    return True
precicely commented 3 months ago

Hi @waltsims, no problem. Will implement as you suggest.

precicely commented 3 months ago

Have now made those changes. That said, and I don't think this is the pull request to do it but perhaps a more elegant solution is to make the __eq__ method of the Element class just work? For example, it would be much nicer to write:

element_1 == element_2

As opposed to the check_element_equality function.

I'm guessing the problem right now is the equality of the numpy arrays... I'm happy to take this on if you think it's worthwhile?

precicely commented 3 months ago

Something like this: https://stackoverflow.com/questions/51743827/how-to-compare-equality-of-dataclasses-holding-numpy-ndarray-boola-b-raises

waltsims commented 3 months ago

That would be a great addition!

waltsims commented 3 months ago

As you said please create a separate PR those changes. Looking forward to it.

precicely commented 3 months ago

Will do. Got wrapped up in other things yesterday.