-
Notifications
You must be signed in to change notification settings - Fork 1
Model component #50
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
base: develop
Are you sure you want to change the base?
Model component #50
Conversation
Added the BSD 3-Clause License to the project.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #50 +/- ##
============================================
+ Coverage 0.00% 98.48% +98.48%
============================================
Files 1 2 +1
Lines 2 396 +394
============================================
+ Hits 0 390 +390
- Misses 2 6 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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 is a great start! No serious issues, the base class for the components is well designed and the derived classes are correctly implemented.
Only minor issues raised:
- missing type annotations (especially for return values)
- naming style consistency
- lack of tests, e.g. no coverage of
copy
,__repr__
,convert_unit
and what's most important - nothing of the base class (apart offix_all_parameters
andfit_all_parameters
@rozyczko would you have a quick look again? I added tons of tests, fixed a few issues that they revealed, and fixed the formatting issues. I removed the user defined component for now, since I need to think more about how I want to implement it, and it's not required for the first release. |
Model components: Gaussian, Lorentzian, Damped Harmonic Oscillator, Delta Function, Polynomial. There is also a user defined component which is not implemented yet - it's there as a reminder.
These components are not intended to be used alone - they should go in SampleModels, which will be in 2 PRs.