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 fitting test (and refactor scripts) #113

Merged
merged 10 commits into from
May 27, 2021
Merged

Conversation

karllark
Copy link
Contributor

@karllark karllark commented Apr 5, 2021

Add in a fitting test. Simply reads in the spitzer scipack and M101 nucleus spectrum, runs the fit, and then compares the fit parameters to a cached array of the parameters. Will fail if the parameters are not close (machine precision I think).

Refactored run_pahfit and plot_pahfit scripts to have common functions to avoid code duplication and allow for easier testing. This refactoring will also make it easier to support other fitters, units, etc. in the future.

@karllark karllark added this to the v2.0 milestone Apr 5, 2021
@karllark karllark marked this pull request as draft April 5, 2021 21:10
@codecov-commenter
Copy link

codecov-commenter commented May 17, 2021

Codecov Report

Merging #113 (6df4037) into master (e7d7f2a) will increase coverage by 21.08%.
The diff coverage is 80.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #113       +/-   ##
===========================================
+ Coverage   54.71%   75.80%   +21.08%     
===========================================
  Files           4        5        +1     
  Lines         265      310       +45     
===========================================
+ Hits          145      235       +90     
+ Misses        120       75       -45     
Impacted Files Coverage Δ
pahfit/base.py 60.89% <0.00%> (+16.96%) ⬆️
pahfit/helpers.py 92.30% <92.30%> (ø)
pahfit/component_models.py 100.00% <0.00%> (+56.75%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e7d7f2a...6df4037. Read the comment docs.

@karllark karllark mentioned this pull request May 17, 2021
@karllark karllark marked this pull request as ready for review May 17, 2021 20:55
Copy link
Contributor

@alexmaragko alexmaragko 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! Perhaps consider some of the changes I propose in my comments, otherwise it seems good as it is, and definitely looks much cleaner now.

pahfit/helpers.py Outdated Show resolved Hide resolved
pahfit/helpers.py Outdated Show resolved Hide resolved
Copy link
Contributor

@els1 els1 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 to me overall.
One question: for 'test_fitting_spitzer.py', you use maxiter=200 (ln 17) while the default is set to 1000. Is this to decrease the time for testing or is this a leftover from the previous setting?
And how do I show that part of the code in this comment (like Alexandros did)?

@karllark
Copy link
Contributor Author

One question: for 'test_fitting_spitzer.py', you use maxiter=200 (ln 17) while the default is set to 1000. Is this to decrease the time for testing or is this a leftover from the previous setting?

For testing so it is fully reproducible regardless of the default maxiter setting. And to reduce the time needed.

@karllark
Copy link
Contributor Author

And how do I show that part of the code in this comment (like Alexandros did)?

If you go to the "Files changed" tab, you can highlight specific lines and add a comment/review. Use your mouse and the "+" symbols.

@els1
Copy link
Contributor

els1 commented May 27, 2021

And how do I show that part of the code in this comment (like Alexandros did)?

If you go to the "Files changed" tab, you can highlight specific lines and add a comment/review. Use your mouse and the "+" symbols.

Thanks.

@karllark
Copy link
Contributor Author

Two reviews enough? I think it is ready to merge.

@els1
Copy link
Contributor

els1 commented May 27, 2021

Two reviews enough? I think it is ready to merge.

Fine with me. But there is one check which was unsuccessful ...

@karllark
Copy link
Contributor Author

But there is one check which was unsuccessful ...

This is codacy being over careful in the default value for a variable in a function definition. It can be ignored.

@els1 els1 merged commit febfa6f into PAHFIT:master May 27, 2021
@karllark karllark mentioned this pull request Jun 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants