Skip to content

Add multifrequency capabilities for ContinuousImage#76

Open
erandichavez wants to merge 34 commits intomainfrom
erandichavez-multifrequency
Open

Add multifrequency capabilities for ContinuousImage#76
erandichavez wants to merge 34 commits intomainfrom
erandichavez-multifrequency

Conversation

@erandichavez
Copy link
Collaborator

Here's the multifrequency code! This is the implementation specifically for ContinuousImage models. This should now work with the TaylorSpectral definition in freqtaylor.jl --- I had to add a new function definition for TaylorSpectral to do this, but everything should still work for the multifrequency geometric modeling!

@codecov
Copy link

codecov bot commented Jan 13, 2025

Codecov Report

Attention: Patch coverage is 93.93939% with 4 lines in your changes missing coverage. Please review.

Project coverage is 91.40%. Comparing base (94bb8d5) to head (0d834d8).
Report is 18 commits behind head on main.

Files with missing lines Patch % Lines
src/models/multifrequency.jl 93.54% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #76      +/-   ##
==========================================
+ Coverage   91.24%   91.40%   +0.16%     
==========================================
  Files          29       30       +1     
  Lines        1975     2059      +84     
==========================================
+ Hits         1802     1882      +80     
- Misses        173      177       +4     

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

@ptiede
Copy link
Member

ptiede commented Jan 13, 2025

For the format suggestions, you can fix those by running JuliaFormatter.format(".") in the VLBISkyModels directory.
Just download JuliaFormatter in your global environment first.

@ptiede
Copy link
Member

ptiede commented Jan 14, 2025

Codecov Report

Attention: Patch coverage is 0% with 64 lines in your changes missing coverage. Please review.

Project coverage is 88.47%. Comparing base (94bb8d5) to head (e77415e).
Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
src/models/multifrequency.jl 0.00% 60 Missing ⚠️
src/models/multidomain/freqtaylor.jl 0.00% 4 Missing ⚠️
Additional details and impacted files
☔ View full report in Codecov by Sentry. 📢 Have feedback on the report? Share it here.

@erandichavez This ensures that the new code functionality is included in unit tests. Tests can be added in the test folder. I think we just want to make sure that intensitymap and visibilitymap work.

@erandichavez
Copy link
Collaborator Author

Just added tests for all the new multifrequency code! It lives in multifrequency.jl - the tests passed on my computer, fingers crossed...

@ptiede
Copy link
Member

ptiede commented Jan 14, 2025

The tests passed. The error is from some weird issue with the GC that I do not understand.

SpecialFunctions = "276daf66-3868-5448-9aa4-cd146d93841b"
StaticArrays = "90137ffa-7385-5640-81b9-e52037218182"
StructArrays = "09ab397b-f2b6-538f-b94a-2f83cf4a842a"
VLBIImagePriors = "b1ba175b-8447-452c-b961-7db2d6f7a029"
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to include VLBIImagePriors as a dep?

# Fields
$(FIELDS)
"""
struct Multifrequency{B<:ContinuousImage,F<:Number,S<:FrequencyParams} <:
Copy link
Member

Choose a reason for hiding this comment

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

This is just an idea and if this is too much work don't worry about it, but how to do you feel about removing this Multifrequency model and just converting your visibility_numeric to work based on ContinuousImage?
So something like replacing

function visibilitymap_numeric(m::Multifrequency, g::AbstractFourierDualDomain)

with

function visibilitymap_numeric(m::ContinuousImage{<:DomainParams}, g::AbstractFourierDualDomain)

and then using the build_params functionality to construct the image grid. This will break all the script, but it will make the code more "consistent".

@ptiede
Copy link
Member

ptiede commented Jan 29, 2025

I think the only thing preventing me from merging this is just adding the missing docstrings to the docs.
The logs for the Documentation build note that

┌ Error: 5 docstrings not included in the manual:
│ 
│     VLBISkyModels.build_imagecube :: Tuple{Multifrequency, RectiGrid}
│     VLBISkyModels.applyspectral :: Union{Tuple{N}, Tuple{AbstractArray, TaylorSpectral, N}} where N<:Number
│     VLBISkyModels.AbstractSpectralModel
│     VLBISkyModels.generatemodel :: Union{Tuple{N}, Tuple{Multifrequency, N}} where N<:Number
│     VLBISkyModels.Multifrequency

if you add those to docs/src/api.md that would be enough!

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@ptiede
Copy link
Member

ptiede commented Jan 29, 2025

Oh and removing VLBIImagePriors as a dep since I did confirm we don't need it.

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.

2 participants