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

Additional options for continuum fit #499

Merged
merged 14 commits into from
Feb 12, 2021

Conversation

marijana777
Copy link
Collaborator

Adding options to:

  • define input and output files with knots in lt_continuumfit
  • define continuum knots automatically, by including only pixels that do not correspond to absorption lines (code: analysis/continuumfnd.py )

@coveralls
Copy link

coveralls commented Feb 28, 2019

Coverage Status

Coverage increased (+0.2%) to 71.544% when pulling a449847 on marijana777:continuumfind into f953c29 on linetools:master.

Copy link
Contributor

@profxj profxj left a comment

Choose a reason for hiding this comment

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

Looks ok overall but the code needs to be more Pythonic.

linetools/analysis/continuumfnd.py Outdated Show resolved Hide resolved
linetools/analysis/continuumfnd.py Show resolved Hide resolved
linetools/analysis/continuumfnd.py Outdated Show resolved Hide resolved
linetools/analysis/continuumfnd.py Outdated Show resolved Hide resolved
- Replaced the two nested for loops with one for loop and np.max. The code is now faster
…The code is also faster now. Tested on two examples before and after changes, and the outputs look the same.
@marijana777
Copy link
Collaborator Author

Addressed all requested changes for now.

@profxj
Copy link
Contributor

profxj commented Jul 2, 2019

Thanks for this @marijana777 .
The only thing left is to add a test or two of your contknots method.
Add it to the tests folder in analysis.

Example spectrum was generated by adding a few Gaussian absorptions to a constant flux and then adding noise.
@marijana777
Copy link
Collaborator Author

Thank you @profxj for the comments. Just added the test of the contknots method

@profxj
Copy link
Contributor

profxj commented Jan 1, 2020

This all looks good now @marijana777
But wait until PR #512 is merged into master
and then pull that in to yours so that Travis tests
should pass. Once they do, I'll approve and merge.

@profxj
Copy link
Contributor

profxj commented Jan 2, 2020

Ok, @marijana777 pull in master and confirm that Travis passes.
THanks.

@marijana777
Copy link
Collaborator Author

@profxj confirming that Travis passed on test_continuumfnd.py in all cases. I also confirmed that the rest of the Travis output seems almost the same as in the most recent merged PR at this moment (i.e., the same tests failed, etc).
Should we merge this PR now?

@profxj profxj merged commit 1d7183c into linetools:master Feb 12, 2021
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.

None yet

3 participants