fold_events keyword validation #837

spranav1205 · 2024-08-29T16:04:32Z

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 · 2024-08-29T16:04:37Z

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 · 2024-08-29T19:46:21Z

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 · 2024-08-30T04:45:45Z

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.

matteobachetti · 2024-08-31T17:32:45Z

stingray/pulse/pulsar.py

    # If no key is passed, *or gti is None*, defaults to the
    # initial and final event
-    gti = _default_value_if_no_key(opts, "gti", None)
+    gti = dict.pop(opts, "gti", None)


Actually, sorry, I think you can just use opts.pop("gti", None)

I've made the changes

codecov · 2024-08-31T17:34:03Z

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 ⚠️

❗ 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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

matteobachetti · 2024-09-11T08:07:52Z

@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 · 2024-09-11T17:42:14Z

@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 · 2024-09-13T06:15:26Z

@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:

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)
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 · 2024-09-13T06:45:31Z

@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

test_plot_profile_errorbars, test_plot_profile_existing_ax

spranav1205 · 2024-09-16T13:53:06Z

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 · 2024-09-16T16:15:11Z

stingray/pulse/pulsar.py

+
+    if opts:
+        raise ValueError(
+            f"Unidentified keys: {opts.keys()} \n Please refer to the description of the function for optional parameters."


Unidentified keyword to fold_events: {opts.keys()} etc. would probably be more informative

matteobachetti · 2024-09-16T18:04:03Z

@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 · 2024-09-18T13:27:21Z

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

matteobachetti

@spranav1205 I suggested a couple of further modifications to the test, below. This makes the error cleaner (just the keys, without the confusing dict_keys syntax).
After this, I will accept and merge.
Thanks for this very useful addition to Stingray!

matteobachetti · 2024-09-18T15:49:00Z

stingray/pulse/pulsar.py

+
+    if opts:
+        raise ValueError(
+            f"Unidentified keyword(s) to fold_events: {opts.keys()} \n Please refer to the description of the function for optional parameters."


Suggested change

f"Unidentified keyword(s) to fold_events: {opts.keys()} \n Please refer to the description of the function for optional parameters."

f"Unidentified keyword(s) to fold_events: {', '.join([k for k in opts.keys()])} \n Please refer to the description of the function for optional parameters."

matteobachetti · 2024-09-18T15:52:40Z

stingray/pulse/tests/test_search.py

@@ -111,6 +111,12 @@ def test_plot_phaseogram_direct(self):
        plt.savefig("phaseogram_direct.png")
        plt.close(plt.gcf())

+    def test_search_wrong_key_fails(self):
+        with pytest.raises(
+            ValueError, match="Unidentified keyword(s) to fold_events: dict_keys(['fdot'])"


Suggested change

ValueError, match="Unidentified keyword(s) to fold_events: dict_keys(['fdot'])"

ValueError, match="Unidentified keyword(s) to fold_events: fdot, fddot"

matteobachetti · 2024-09-18T15:52:56Z

stingray/pulse/tests/test_search.py

+        with pytest.raises(
+            ValueError, match="Unidentified keyword(s) to fold_events: dict_keys(['fdot'])"
+        ):
+            phase, prof, _ = fold_events(self.event_times, self.pulse_frequency, fdot=12)


Suggested change

phase, prof, _ = fold_events(self.event_times, self.pulse_frequency, fdot=12)

phase, prof, _ = fold_events(self.event_times, self.pulse_frequency, fdot=12, fddot=34)

spranav1205 · 2024-09-18T16:44:22Z

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

matteobachetti

The match regex isn't working, probably better to simplify it to the point it's specific (it contains the correct keywords) but it does not contain problematic characters (e.g. escapes)

matteobachetti · 2024-09-18T19:41:10Z

stingray/pulse/tests/test_search.py

+    def test_search_wrong_key_fails(self):
+        with pytest.raises(
+            ValueError,
+            match="Unidentified keyword(s) to fold_events: fdot, fddot \n Please refer to the description of the function for optional parameters.",


Suggested change

match="Unidentified keyword(s) to fold_events: fdot, fddot \n Please refer to the description of the function for optional parameters.",

match="Unidentified keyword(s) to fold_events: fdot, fddot",

Its better to match the smallest possible part of the regex that makes it really specific. Try to run it locally in your computer, with the --run-slow option, to verify that it works

The brackets were causing that issue. I fixed it and the test is working on my PC.
Two checks are failing. It says the tests do not cover line 281.
Line 281 is raise ValueError(. But the test case covers it so I am not sure why the check is failing.

matteobachetti

Thanks @spranav1205 !

Event_folding key validation

b5f9ca1

spranav1205 mentioned this pull request Aug 29, 2024

fold_events accepts keywords and ignores them #835

Closed

Formatting

d6ef817

spranav1205 added 3 commits August 30, 2024 18:54

Attempting to use dict.pop()

13646d8

Formatting 2.0

638afec

Editted value error message

d5806c4

matteobachetti reviewed Aug 31, 2024

View reviewed changes

spranav1205 added 2 commits September 2, 2024 18:04

opts.pop instead of dict.pop

470ea84

Formating 3.0

5fa9667

Changelog updated

327e9eb

Removing ax as a kwarg from test cases:

68a2931

test_plot_profile_errorbars, test_plot_profile_existing_ax

matteobachetti reviewed Sep 16, 2024

View reviewed changes

spranav1205 added 2 commits September 18, 2024 18:45

Updating error message and adding test case

dff319a

fixing typo

d6af584

matteobachetti requested changes Sep 18, 2024

View reviewed changes

spranav1205 added 2 commits September 18, 2024 21:44

Final Changes

d760376

Formatting

035a77d

Resolving issue

a40ebfb

Formatting

efefb48

matteobachetti requested changes Sep 18, 2024

View reviewed changes

Regex error fixed

e5f0653

matteobachetti enabled auto-merge September 19, 2024 17:51

matteobachetti approved these changes Sep 19, 2024

View reviewed changes

matteobachetti added this pull request to the merge queue Sep 19, 2024

Merged via the queue into StingraySoftware:main with commit 0b1783a Sep 19, 2024
15 of 17 checks passed

spranav1205 deleted the Event_folding_#835 branch September 20, 2024 04:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fold_events keyword validation #837

fold_events keyword validation #837

spranav1205 commented Aug 29, 2024

pep8speaks commented Aug 29, 2024 •

edited

Loading

matteobachetti commented Aug 29, 2024

spranav1205 commented Aug 30, 2024

matteobachetti Aug 31, 2024

spranav1205 Sep 5, 2024

codecov bot commented Aug 31, 2024 •

edited

Loading

matteobachetti commented Sep 11, 2024 •

edited

Loading

spranav1205 commented Sep 11, 2024

spranav1205 commented Sep 13, 2024

matteobachetti commented Sep 13, 2024 •

edited

Loading

spranav1205 commented Sep 16, 2024

matteobachetti Sep 16, 2024

matteobachetti commented Sep 16, 2024

spranav1205 commented Sep 18, 2024

matteobachetti left a comment

matteobachetti Sep 18, 2024

matteobachetti Sep 18, 2024

matteobachetti Sep 18, 2024

spranav1205 commented Sep 18, 2024

matteobachetti left a comment

matteobachetti Sep 18, 2024

spranav1205 Sep 19, 2024 •

edited

Loading

matteobachetti left a comment

	f"Unidentified keyword(s) to fold_events: {opts.keys()} \n Please refer to the description of the function for optional parameters."
	f"Unidentified keyword(s) to fold_events: {', '.join([k for k in opts.keys()])} \n Please refer to the description of the function for optional parameters."

	ValueError, match="Unidentified keyword(s) to fold_events: dict_keys(['fdot'])"
	ValueError, match="Unidentified keyword(s) to fold_events: fdot, fddot"

	phase, prof, _ = fold_events(self.event_times, self.pulse_frequency, fdot=12)
	phase, prof, _ = fold_events(self.event_times, self.pulse_frequency, fdot=12, fddot=34)

	match="Unidentified keyword(s) to fold_events: fdot, fddot \n Please refer to the description of the function for optional parameters.",
	match="Unidentified keyword(s) to fold_events: fdot, fddot",

fold_events keyword validation #837

fold_events keyword validation #837

Conversation

spranav1205 commented Aug 29, 2024

Relevant Issue(s)/PR(s)

Overview of Implemented Solution

Description

pep8speaks commented Aug 29, 2024 • edited Loading

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

matteobachetti commented Aug 29, 2024

spranav1205 commented Aug 30, 2024

matteobachetti Aug 31, 2024

Choose a reason for hiding this comment

spranav1205 Sep 5, 2024

Choose a reason for hiding this comment

codecov bot commented Aug 31, 2024 • edited Loading

Codecov Report

matteobachetti commented Sep 11, 2024 • edited Loading

spranav1205 commented Sep 11, 2024

spranav1205 commented Sep 13, 2024

matteobachetti commented Sep 13, 2024 • edited Loading

spranav1205 commented Sep 16, 2024

matteobachetti Sep 16, 2024

Choose a reason for hiding this comment

matteobachetti commented Sep 16, 2024

spranav1205 commented Sep 18, 2024

matteobachetti left a comment

Choose a reason for hiding this comment

matteobachetti Sep 18, 2024

Choose a reason for hiding this comment

matteobachetti Sep 18, 2024

Choose a reason for hiding this comment

matteobachetti Sep 18, 2024

Choose a reason for hiding this comment

spranav1205 commented Sep 18, 2024

matteobachetti left a comment

Choose a reason for hiding this comment

matteobachetti Sep 18, 2024

Choose a reason for hiding this comment

spranav1205 Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

matteobachetti left a comment

Choose a reason for hiding this comment

pep8speaks commented Aug 29, 2024 •

edited

Loading

codecov bot commented Aug 31, 2024 •

edited

Loading

matteobachetti commented Sep 11, 2024 •

edited

Loading

matteobachetti commented Sep 13, 2024 •

edited

Loading

spranav1205 Sep 19, 2024 •

edited

Loading