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

Add a unit test for dyn bgd hotpix imposter calc #418

Merged
merged 7 commits into from
Jul 11, 2023
Merged

Conversation

jeanconn
Copy link
Contributor

Description

Add a unit test for dyn bgd hotpix imposter calc. Maybe it is a bridge too far for the #412 PR, but we could start to accumulate Python unit tests here as we go. This is not a particularly clean unit test; I suppose it would be better to ask sparkles what we want for dyn_bgd_n_faint and dyn_bgd_dt_ccd or some such? But this does work as a functional test for the imposter calc.

This addresses the issue at the end of #412 that the functional testing of the imposter calculation was not demonstrated.

Interface impacts

Testing

I tested this with install testing into my ska3-matlab-2023.4rc6 environment.

Unit tests

  • Mac

Independent check of unit tests by [REVIEWER NAME]

  • [PLATFORM]:

Functional tests

No functional testing.

@jeanconn jeanconn requested a review from taldcroft July 10, 2023 18:58
@jeanconn
Copy link
Contributor Author

@taldcroft I don't know if we actually want to merge this or something like it, but it seemed a reasonable way to document what I did for functional test.

@jeanconn jeanconn mentioned this pull request Jul 11, 2023
1 task
Copy link
Member

@taldcroft taldcroft left a comment

Choose a reason for hiding this comment

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

This looks great! There was only one real issue with using >= or <= instead of splitting to > and ==. Then I saw an opportunity to simplify the code and started typing.

starcheck/tests/test_utils.py Outdated Show resolved Hide resolved
starcheck/tests/test_utils.py Outdated Show resolved Hide resolved
starcheck/tests/test_utils.py Outdated Show resolved Hide resolved
starcheck/tests/test_utils.py Outdated Show resolved Hide resolved
jeanconn and others added 4 commits July 11, 2023 07:46
Co-authored-by: Tom Aldcroft <taldcroft@gmail.com>
Co-authored-by: Tom Aldcroft <taldcroft@gmail.com>
Co-authored-by: Tom Aldcroft <taldcroft@gmail.com>
Co-authored-by: Tom Aldcroft <taldcroft@gmail.com>
@jeanconn
Copy link
Contributor Author

OK. Thanks. I just committed your changes and re-ran the test successfully.

@jeanconn jeanconn requested a review from taldcroft July 11, 2023 11:56
@jeanconn
Copy link
Contributor Author

This is a PR against #412, so my preference would be to get #412 approved without this, get this approved, and then merge them both.

Copy link
Member

@taldcroft taldcroft left a comment

Choose a reason for hiding this comment

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

Looks good now. I ran the test and got a pass. Not required, but probably worth cleaning up these warnings by preceding the regex string with r.

============================================= warnings summary ==============================================
starcheck/pcad_att_check.py:51
  /Users/aldcroft/git/starcheck/starcheck/pcad_att_check.py:51: DeprecationWarning: invalid escape sequence '\d'
    match = re.match('^(\d+\.\d+)\s+\|\s+(\S+)\s*$',

starcheck/pcad_att_check.py:67
  /Users/aldcroft/git/starcheck/starcheck/pcad_att_check.py:67: DeprecationWarning: invalid escape sequence '\d'
    match = re.match('^(\d+\.\d+)\s+\|\s+(\S+)\s+(\S+)\s+(\S+)\s+(\S+)\s*', line)

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html

@jeanconn
Copy link
Contributor Author

Thanks! And I'm fine with making those regex changes if we are fine with a "starcheck still seemed to run ok" test for the record as enough testing for that.

@jeanconn
Copy link
Contributor Author

Though actually... what are you doing to see those pcad_att_check warnings?

@jeanconn jeanconn merged commit 6a4b4dd into bonus Jul 11, 2023
@jeanconn jeanconn deleted the hotpix-unit-test branch July 11, 2023 15:07
@javierggt javierggt mentioned this pull request Aug 9, 2023
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