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

Fitter legacy #137

Merged
merged 9 commits into from
Feb 21, 2024
Merged

Fitter legacy #137

merged 9 commits into from
Feb 21, 2024

Conversation

KriSun95
Copy link
Collaborator

Changing sunxspex_fitting to fitting_legacy and adding new fitting module.

@@ -0,0 +1 @@
Rename of `sunxspex_fitting` module tp `fitting_legacy` and added new `fitting` module.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Rename of `sunxspex_fitting` module tp `fitting_legacy` and added new `fitting` module.
Rename of `sunxspex_fitting` module to `fitting_legacy` and added new `fitting` module.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also the new fitter module hasn't been added which is fine but if not going to do in this PR then probably remove from change log.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't realise empty directories did not get added. I have now managed to get the fitting module to be included.

Copy link
Member

@DanRyanIrish DanRyanIrish left a comment

Choose a reason for hiding this comment

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

Approved, pending changelog chance, pulling latest main branch, and all tests passing.

@codecov-commenter
Copy link

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

❗ No coverage uploaded for pull request base (main@2257afb). Click here to learn what that means.

Files Patch % Lines
sunkit_spex/fitting_legacy/fitter.py 88.88% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #137   +/-   ##
=======================================
  Coverage        ?   55.81%           
=======================================
  Files           ?       20           
  Lines           ?     3166           
  Branches        ?        0           
=======================================
  Hits            ?     1767           
  Misses          ?     1399           
  Partials        ?        0           

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

Copy link
Collaborator

@samaloney samaloney left a comment

Choose a reason for hiding this comment

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

Tests are no in the green! This looks good to me once the change log is fixed up.

@KriSun95
Copy link
Collaborator Author

I've made changes to the changelog and also added the fitting module. Thanks to @samaloney for handling the update to match main!

@samaloney samaloney merged commit 2deabc0 into sunpy:main Feb 21, 2024
12 checks passed
KriSun95 added a commit to KriSun95/sunkit-spex that referenced this pull request Mar 4, 2024
* Changed fittter_legacy module to fitting_legacy and added new fitting module

---------

Co-authored-by: Kristopher Cooper <[email protected]>
Co-authored-by: Shane Maloney <[email protected]>
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.

4 participants