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

compatiblity updates with HARPS and HARPS-N spectra #12

Merged
merged 6 commits into from
May 23, 2021

Conversation

alexander-wise
Copy link
Contributor

support for km/s RV shifts and a binary RV shift

@codecov-commenter
Copy link

codecov-commenter commented Apr 30, 2021

Codecov Report

Merging #12 (38b2588) into main (93650e7) will decrease coverage by 0.37%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #12      +/-   ##
==========================================
- Coverage   31.00%   30.63%   -0.38%     
==========================================
  Files          19       19              
  Lines         987      999      +12     
==========================================
  Hits          306      306              
- Misses        681      693      +12     
Impacted Files Coverage Δ
src/types/spectra.jl 46.66% <0.00%> (-7.18%) ⬇️
src/util/physics.jl 9.21% <0.00%> (-0.13%) ⬇️
src/util/spectra.jl 18.18% <0.00%> (-1.05%) ⬇️

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 93650e7...38b2588. Read the comment docs.

@@ -55,20 +55,23 @@ function apply_doppler_boost!(spectra::AS, dict::AbstractDict ) where { AS<:Abst
doppler_factor *= calc_doppler_factor(dict[:drift_rv])
end
=#
if !haskey(dict,:ssb_rv) && !haskey(dict,:ssbz) && !have_issued_ssb_warning
if !haskey(dict,:ssb_rv) && !haskey(dict,:ssbz) && !haskey(dict,:ssb_rv_kmps) && !have_issued_ssb_warning
Copy link
Member

Choose a reason for hiding this comment

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

If we're going to allow either of these, then let's add a check that both aren't set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated this so a warning is printed if both are set, so the user knows which is used by default.

@assert 1 <= min_pixel_in_order(inst) <= max_pixel_in_order(inst) <= size(λ,1)
@assert 1 <= min_order(inst) <= max_order(inst) <= size(λ,2)
@assert 1 <= size(λ,1) <= max_pixel_in_order(inst)
@assert 1 <= size(λ,2) <= max_order(inst)
Spectra2DBasic{eltype(λ),eltype(flux),eltype(var),typeof(λ),typeof(flux),typeof(var),typeof(inst)}(λ,flux,var,inst,metadata)
Copy link
Member

Choose a reason for hiding this comment

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

I agree max_... should be less than size... But why not leave the check for min... being bigger than 1 and less than max?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can add separate line that min_order is between 1 and max_order

Copy link
Member

@eford eford left a comment

Choose a reason for hiding this comment

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

Just a few minor suggestions. Please let me know if it would be helpful to discuss any.

Copy link
Member

@eford eford left a comment

Choose a reason for hiding this comment

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

Thanks

@eford eford merged commit 91f5756 into RvSpectML:main May 23, 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.

3 participants