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 multifrequency capabilities for ContinuousImage #76

Open
wants to merge 33 commits into
base: main
Choose a base branch
from

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!

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.

@@ -30,6 +30,7 @@ Serialization = "9e88b42a-f829-5b0c-bbe9-9e923198166b"
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?

docs/src/interface.md Outdated Show resolved Hide resolved
docs/src/interface.md Outdated Show resolved Hide resolved
docs/src/interface.md Outdated Show resolved Hide resolved
src/models/multifrequency.jl Outdated Show resolved Hide resolved
# 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".

src/models/multifrequency.jl Outdated Show resolved Hide resolved
@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