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

Implement GTI Reading and Handling #3

Merged
merged 2 commits into from Aug 8, 2022
Merged

Conversation

Aman-Pandey-afk
Copy link
Collaborator

@Aman-Pandey-afk Aman-Pandey-afk commented Jul 23, 2022

GTI machinery and reading from file is implemented here, tests are passing and garbage collection is near to 0. Some points I would like to mention:

  • Have used interconversion between Interval, Vectors and Matrices at many places which maybe less efficient but were required (like using eachrow sometimes which makes traversal in row order possible, which is inefficient as Julia matrices are stored in column-major order)
  • I have once used nothing (in the operations_on_gti function) as it seemed convenient and efficient to me instead of assigning the required_interval an IntervalSet using the first element of gti_list.
  • I removed type instabilities in most of the functions, but one seems to persist -: setdiff(IntervalSet1, IntervalSet2). I looked ways to remove it but can't, it outputs an IntervalSet of AbstractIntervals.
  • The throw(ArgumentError) is showing Any Type
  • Also, in some of the functions mapreduce can give error if empty vectors are passed. Should I create appropriate Error throws for them?
  • The PR is based on Add fourier methods and tests #2 . So it should be merged before this one if there are no problems except the refactoring.

@codecov
Copy link

codecov bot commented Jul 23, 2022

Codecov Report

Merging #3 (fc9473e) into aman-fourier (53d8ffe) will increase coverage by 0.69%.
The diff coverage is 97.50%.

@@               Coverage Diff                @@
##           aman-fourier       #3      +/-   ##
================================================
+ Coverage         94.55%   95.25%   +0.69%     
================================================
  Files                 3        3              
  Lines               386      506     +120     
================================================
+ Hits                365      482     +117     
- Misses               21       24       +3     
Impacted Files Coverage Δ
src/utils.jl 92.30% <88.88%> (-7.70%) ⬇️
src/gti.jl 93.12% <98.19%> (+11.49%) ⬆️

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 53d8ffe...fc9473e. Read the comment docs.

src/utils.jl Outdated
function contiguous_regions(condition::AbstractVector{Bool})
# Find the indicies of changes in "condition"
d = diff(condition)
idx = findall(x->x != 0, d)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
idx = findall(x->x != 0, d)
idx = findall(!iszero, d)

src/utils.jl Outdated

if condition[1]
# If the start of condition is True prepend a 0
idx = append!([1],idx)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you wanted to use pushfirst!

Suggested change
idx = append!([1],idx)
pushfirst!(idx, 1)

src/utils.jl Outdated

if condition[end]
# If the end of condition is True, append the length of the array
idx = append!(idx,[condition.size+1]) # Edit
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
idx = append!(idx,[condition.size+1]) # Edit
push!(idx, condition.size + 1) # Edit

What does "edit" mean?

src/gti.jl Outdated
))
end

if any(@view(gti_start[2:end]) < @view(gti_end[1:end-1]))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if any(@view(gti_start[2:end]) < @view(gti_end[1:end-1]))
if any(@view(gti_start[begin+1:end]) < @view(gti_end[begin:end-1]))

src/gti.jl Outdated
new_gtis[ig][:] .= limmin, limmax
for (i,t) in enumerate(times)
if t >= (limmin + dt / 2 - epsilon_times_dt) && t <= (limmax - dt / 2 + epsilon_times_dt)
mask[i] = true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Indentation is off

src/gti.jl Outdated
safe_interval::AbstractVector{<:Real}=[0,0], min_length::Real=0,
dt::Real = -1, epsilon::Real = 0.001)

if length(times) == 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if length(times) == 0
if isempty(times)

should be the same, right?

src/gti.jl Outdated
end

function get_btis(gtis::AbstractMatrix{<:Real})
if length(gtis) == 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if length(gtis) == 0
if isempty(gtis)

src/gti.jl Outdated
end

function get_btis(gtis::AbstractMatrix{T}, start_time, stop_time) where {T<:Real}
if length(gtis) == 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if length(gtis) == 0
if isempty(gtis)


function get_btis(gtis::AbstractMatrix{T}, start_time, stop_time) where {T<:Real}
if length(gtis) == 0
return T[start_time stop_time]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you return a matrix instead of a vector on purpose?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah to return same types

src/gti.jl Outdated
Comment on lines 7 to 10
lchdulist = FITS(fits_file)
gtihdu = lchdulist[gtistring]
gti = get_gti_from_hdu(gtihdu)
close(lchdulist)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can do

Suggested change
lchdulist = FITS(fits_file)
gtihdu = lchdulist[gtistring]
gti = get_gti_from_hdu(gtihdu)
close(lchdulist)
gti = FITS(fits_file) do lchdulist
gtihdu = lchdulist[gtistring]
get_gti_from_hdu(gtihdu)
end

@codecov-commenter
Copy link

codecov-commenter commented Aug 2, 2022

Codecov Report

❗ No coverage uploaded for pull request base (aman-fourier@53d8ffe). Click here to learn what that means.
The diff coverage is n/a.

@@               Coverage Diff               @@
##             aman-fourier       #3   +/-   ##
===============================================
  Coverage                ?   95.24%           
===============================================
  Files                   ?        3           
  Lines                   ?      505           
  Branches                ?        0           
===============================================
  Hits                    ?      481           
  Misses                  ?       24           
  Partials                ?        0           

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@matteobachetti matteobachetti merged commit f987dde into aman-fourier Aug 8, 2022
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

4 participants