fold_events keyword validation

[spranav1205]

Relevant Issue(s)/PR(s)

Issue: fold_events accepts keywords and ignores them (#835)

Overview of Implemented Solution

I implemented a function to verify if the keywords passed to fold_events match the optional parameters.

Description

I created a function called validate_key_in_list. This function takes two arguments: key and optional_parameters_list. It checks if the key is present in the optional_parameters_list. If the key is not found in the list, the user receives a warning. Additionally, I have included an optional ValueError, which is commented out for now.

Note: This is my first contribution to the project. 😊

[pep8speaks]

Hello @spranav1205! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

• In the file stingray/pulse/pulsar.py:

Line 282:101: E501 line too long (168 > 100 characters)

Comment last updated at 2024-09-19 13:09:28 UTC

[matteobachetti]

Hello @spranav1205, thanks a lot for your contribution! Although this solution is pretty neat, I think there is a leaner solution to this, that we use in other parts of the code but did not get around to use here. You PR is a good opportunity to refresh some of this old code! Unbeknownst to me when I wrote this piece of code, the dict.pop() method has an optional argument which gives the default value. So, the series of x = _default_value_if_no_key(opts, key, default) might be handily replaced by a series of x = opts.pop(key, default). After you have done all calls to pop, the opts dictionary should be empty. If not, you can raise a ValueError with the offending keys. You would miss the list of available keywords, but they are already listed in the docstring in any case. What do you think of this alternative strategy?

[spranav1205]

Yes, I believe this approach would work well, and it would definitely be more systematic. Should I go ahead and implement it?

Additionally, I noticed that fdot (frequency derivatives) are passed to the function as part of the *args and is used later on as well. It's also mentioned in the docstring. This might be unclear to users, so we might consider providing an example of how to use fdot in the docstring to clarify its usage, if necessary.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 90.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 78.76%. Comparing base (241f81a) to head (e5f0653). Report is 23 commits behind head on main.

Files with missing lines	Patch %	Lines
stingray/pulse/pulsar.py	90.00%	1 Missing :warning:

:exclamation: There is a different number of reports uploaded between BASE (241f81a) and HEAD (e5f0653). Click for more details.

HEAD has 85 uploads less than BASE

Flag	BASE (241f81a)	HEAD (e5f0653)
	86	1

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #837       +/-   ##
===========================================
- Coverage   96.53%   78.76%   -17.78%     
===========================================
  Files          48       48               
  Lines        9257     9285       +28     
===========================================
- Hits         8936     7313     -1623     
- Misses        321     1972     +1651     

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

[matteobachetti]

@spranav1205 there is a small remaining issue that makes some tests fail, do you thing you can address it? Also, small thing: the PR is missing a changelog entry. Please look at https://docs.stingray.science/en/stable/contributing.html#updating-and-maintaining-the-changelog

[spranav1205]

@spranav1205 there is a small remaining issue that makes some tests fail, do you thing you can address it? Also, small thing: the PR is missing a changelog entry. Please look at https://docs.stingray.science/en/stable/contributing.html#updating-and-maintaining-the-changelog

Yes, I'll look into that. And the other thing too.

[spranav1205]

@matteobachetti I noticed that the failing test cases are due to the ax parameter being passed as a keyword argument. The solution to this issue would be to either:

1. Assign a Plot: If ax is not None, make sure to assign the plot to ax. (Currently ax is not being used by fold_events)
2. Revise the Testing Code: Update the test cases by removing the ax parameter.

You can review the relevant test code here: GitHub Permalink.

What would you like me to do?

Also, I made the changelog. Please let me know if there's any mistake or anything else I should take care of while contributing.

[matteobachetti]

@spranav1205 If I read this correctly, the ax argument is not used inside fold_events, so it was probably a mistake to have it there in the first place, and your PR allowed to find the mistake in our tests, which is great ;) Please remove the ax keyword argument from the fold_events call, and everything should work fine

[spranav1205]

I have addressed the test cases. I think it should be fine now. Also if there are any more issues or areas that need to be looked at I'd be eager to contribute!

[matteobachetti]

@spranav1205 one last request: a test that the error is really raised, to be added to test_search.py. Something like

    def test_search_wrongk_key_fails(self):
        with pytest.raises(ValueError, match="Unidentified keys: "):
            fold_events(..., fdot=4)

Here, pytest.raises should match the error as specifically as possible. There are many examples in the code.

[spranav1205]

I have made the changes. Should I add another test or a more generic one, or does this suffice?

[spranav1205]

I made the changes. Thanks for letting me work on this, it was my first open-source contribution. You've been a great help.