-
Notifications
You must be signed in to change notification settings - Fork 25
Unit test coverage #864
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
Unit test coverage #864
Conversation
The _copy_input_if_needed function does not work unless it's float64. Is that a problem?
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #864 +/- ##
===========================================
+ Coverage 56.19% 68.82% +12.63%
===========================================
Files 143 144 +1
Lines 22069 22090 +21
===========================================
+ Hits 12401 15203 +2802
+ Misses 9668 6887 -2781 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| return output | ||
|
|
||
|
|
||
| # TODO: This function does not work when array or kernel are 32-bit float types. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function was copied from astropy. It does tend to be one of the more time-consuming functions that run for things like SNIP algorithms. If astropy ever makes significant performance improvements to their code, we may want to copy it again from. Here is the latest version.
We might should avoid from deviating too far from their code, since we may rely on them for updates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a good reason for us to have our own version of this? Would it not make more sense to simply use their version instead of an old copy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think astropy has too many dependencies, and if we add them as a dependency we end up requiring all of their dependencies too.
This module was relatively stand-alone so it was easier to copy over.
|
|
||
| @active_beam_name.setter | ||
| def active_beam_name(self, name: str): | ||
| if self._active_beam_name not in self.beam_dict: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch! I actually think this might fix a GUI error we've seen.
| det = instr.detectors['det1'] | ||
| det.tvec = np.array([0.0, 0.0, -1000.0]) | ||
| instr.tvec = np.zeros(3) | ||
| type(det).pixel_area = PropertyMock(return_value=1.0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The detector's pixel_area attribute is being modified here.
psavery
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
Start to add unit tests to core, slight improvement in Exception management |
Overview
This PR adds unit test coverage for broad swaths of the HEXRD core section. It does not offer complete coverage, but is a stepping stone in the path to 100% library coverage. Upon merge, this will bring total coverage up to 67%.
The only changes to the library code itself amount to fixing which Exceptions are thrown in error cases. Previously, there were assert checks and incorrect RuntimeErrors thrown, which have been fixed for production use to ValueError and TypeError handling instead.
Affected Workflows
This PR does not affect end users, but does increase the amount of unit test coverage the HEXRD library has for developer use.
Documentation Changes
No changes to documentation, as the places where there were changes to the library itself were not documented prior. This amounts to only fixes for the Exception types that are thrown.