DASDAE / dascore

A python library for distributed fiber optic sensing
Other
80 stars 17 forks source link

Raise spool.viz #425

Closed ahmadtourei closed 2 months ago

ahmadtourei commented 2 months ago

Description

This PR implements a detailed AttributeError when Spool.viz is used, directing users to use Patch.viz instead for data visualization.

Checklist

I have (if applicable):

d-chambers commented 2 months ago

Thanks for opening a new PR @ahmadtourei.

Ok, so I didn't understand why raising a custom AttributeError when a spool.viz is accessed is not sufficient here?

eg

@property
def viz(self):
    raise AttributeError("spool has no viz namespace, perhaps you meant Patch.viz?")

Maybe you are envisioning adding spool visualization methods? In that case we might need to support the spool.viz namespace and just raise when .waterfall is accessed to address the mistake in the dascore diagram?

ahmadtourei commented 2 months ago

Thanks for opening a new PR @ahmadtourei.

Ok, so I didn't understand why raising a custom AttributeError when a spool.viz is accessed is not sufficient here?

eg

@property
def viz(self):
    raise AttributeError("spool has no viz namespace, perhaps you meant Patch.viz?")

Maybe you are envisioning adding spool visualization methods? In that case we might need to support the spool.viz namespace and just raise when .waterfall is accessed to address the mistake in the dascore diagram?

Agreed. The problem was the test I implemented. Please have a look at the new test and let me know if it looks good.

d-chambers commented 2 months ago

The test failures are a problem with the macos scipy build and nothing related to your changes. I will try to take a look tomorrow

codecov[bot] commented 2 months ago

Codecov Report

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

Project coverage is 99.84%. Comparing base (cfce558) to head (5b9c56e). Report is 2 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #425 +/- ## ========================================== + Coverage 99.28% 99.84% +0.56% ========================================== Files 112 112 Lines 9028 9032 +4 ========================================== + Hits 8963 9018 +55 + Misses 65 14 -51 ``` | [Flag](https://app.codecov.io/gh/DASDAE/dascore/pull/425/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DASDAE) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/DASDAE/dascore/pull/425/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=DASDAE) | `99.84% <100.00%> (+0.56%)` | :arrow_up: | 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=DASDAE#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.

ahmadtourei commented 2 months ago

The test failures are a problem with the macos scipy build and nothing related to your changes. I will try to take a look tomorrow

Thanks for the discussion on debugging test failures today! All tests pass.