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

Update pulsar.filter_data #409

Merged
merged 9 commits into from
Mar 18, 2025
Merged

Update pulsar.filter_data #409

merged 9 commits into from
Mar 18, 2025

Conversation

blarsen10
Copy link
Contributor

  • Fixes a missing underscore, which previously caused planetssb to not update properly
  • Allows option to input custom mask if not doing a simple time-slice
  • Improves docstring

Copy link

codecov bot commented Jan 31, 2025

Codecov Report

Attention: Patch coverage is 16.66667% with 5 lines in your changes missing coverage. Please review.

Project coverage is 71.53%. Comparing base (0d561cd) to head (e82c676).
Report is 77 commits behind head on dev.

Files with missing lines Patch % Lines
enterprise/pulsar.py 16.66% 5 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #409      +/-   ##
==========================================
+ Coverage   71.50%   71.53%   +0.03%     
==========================================
  Files          13       13              
  Lines        3246     3246              
==========================================
+ Hits         2321     2322       +1     
+ Misses        925      924       -1     
Files with missing lines Coverage Δ
enterprise/pulsar.py 28.57% <16.66%> (ø)

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 40060b1...e82c676. Read the comment docs.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

end_time (float, optional): End time (MJD) for filtering. Ignored if `mask` is provided. Default None.
"""
if mask is None:
if start_time is None and end_time is None:
Copy link
Member

Choose a reason for hiding this comment

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

Maybe make this if-statement flat?

So

if mask is not None:
    # do stuff
elif start_time is not None or end_time is not None:
    start_time is start_time if start_time is not None else np.min(self._toas)
    end_time is # etc
else:
    # mask = np.ones(self._toas.shape, dtype=bool)

Copy link
Member

Choose a reason for hiding this comment

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

@blarsen10

To avoid having to re-write tests, you could get rid of if-statements by starting with

start_time is start_time if start_time is not None else np.min(self._toas)
mask = np.ones(self._toas.shape, dtype=bool)
mask[self._toas < start_time] = False
# etc

@vhaasteren
Copy link
Member

Hi @blarsen10 ,

Can you merge dev or master and push your branch? If you then also address above comment, I will merge it.

Thanks!

@vhaasteren
Copy link
Member

vhaasteren commented Mar 14, 2025

Looks like this function was not covered in tests in the first place, so number of lines tested stays the same. I'm happy to merge like this.

@blarsen10 , perhaps you can respond to that last question? Seems like we could allow combining the options

Copy link
Member

@vhaasteren vhaasteren left a comment

Choose a reason for hiding this comment

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

I'm happy to merge. But, perhaps we want to consider whether we want to be able to combine mask and start_time and end_time?


start_time = start_time * 86400 if start_time is not None else np.min(self._toas)
end_time = end_time * 86400 if end_time is not None else np.max(self._toas)
mask = mask if mask is not None else np.logical_and(self._toas >= start_time, self._toas <= end_time)
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to combine, with a logical and, the mask that was given, and the mask from start_time and end_time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that's a good idea, I updated this using your suggestion below!

@vhaasteren
Copy link
Member

Hi @blarsen10 ,

I would propose to modify it to something like the below. Then the and/or in the docstring makes sense:

def filter_data(self, mask=None, start_time=None, end_time=None):
    """
    Filters the dataset to create a time-slice based on a custom mask and/or a time range.
    
    Parameters:
        mask (array-like, optional): Boolean array specifying which data to keep.
                                     If provided, this mask will be combined (logical AND)
                                     with the time range filter. Default is None.
        start_time (float, optional): Start time (MJD) for filtering. If None, the minimum time in the dataset is used.
        end_time (float, optional): End time (MJD) for filtering. If None, the maximum time in the dataset is used.
    """
    
    start_time = start_time * 86400 if start_time is not None else np.min(self._toas)
    end_time = end_time * 86400 if end_time is not None else np.max(self._toas)
    mask_times = np.logical_and(self._toas >= start_time, self._toas <= end_time)
    mask = np.logical_and(mask, mask_times) if mask is not None else mask_times

@vhaasteren vhaasteren merged commit a536b06 into nanograv:dev Mar 18, 2025
14 of 15 checks passed
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