Added a screenshot field to the ProfileData structure #176
Conversation
To allow screenshots to be saved in ProfileData, I added this field. I also added the respective function create_profile_screenshot! and adapted extract_ProfileData. Tests were also modified.
|
Your PR requires formatting changes to meet the project's style guidelines. Click here to view the suggested changes.diff --git a/src/ProfileProcessing.jl b/src/ProfileProcessing.jl
index 27476b0..037ff1c 100644
--- a/src/ProfileProcessing.jl
+++ b/src/ProfileProcessing.jl
@@ -299,13 +299,13 @@ function create_profile_screenshot!(Profile::ProfileData, DataSet::NamedTuple)
x_profile = flatten_cross_section(data_tmp, Start = Profile.start_lonlat) # compute the distance along the profile
data_tmp = addfield(data_tmp, "x_profile", x_profile)
- # add the data set as a NamedTuple
+ # add the data set as a NamedTuple
data_NT = NamedTuple{(DataSetName[idata],)}((data_tmp,))
tmp = merge(tmp, data_NT)
else
# we do not have this implemented
#error("horizontal profiles not yet implemented")
- end
+ end
end
Profile.SurfData = tmp # assign to profile data structure
return
@@ -394,7 +394,7 @@ end
Extracts data along a vertical or horizontal profile
"""
-function extract_ProfileData!(Profile::ProfileData, VolData::Union{Nothing, GeoData} = nothing, SurfData::NamedTuple = NamedTuple(), PointData::NamedTuple = NamedTuple(),ScreenshotData::NamedTuple = NamedTuple(); DimsVolCross = (100, 100), Depth_extent = nothing, DimsSurfCross = (100,), section_width = 50km)
+function extract_ProfileData!(Profile::ProfileData, VolData::Union{Nothing, GeoData} = nothing, SurfData::NamedTuple = NamedTuple(), PointData::NamedTuple = NamedTuple(), ScreenshotData::NamedTuple = NamedTuple(); DimsVolCross = (100, 100), Depth_extent = nothing, DimsSurfCross = (100,), section_width = 50km)
if !isnothing(VolData)
create_profile_volume!(Profile, VolData; DimsVolCross = DimsVolCross, Depth_extent = Depth_extent)
@@ -415,13 +415,12 @@ Extracts data along a vertical or horizontal profile.
function extract_ProfileData!(Profile::ProfileData, VolData::Union{Nothing, GeoData} = nothing, SurfData::NamedTuple = NamedTuple(), PointData::NamedTuple = NamedTuple(); DimsVolCross = (100, 100), Depth_extent = nothing, DimsSurfCross = (100,), section_width = 50km)
# call the actual function to extract the profile data
- extract_ProfileData!(Profile, VolData, SurfData, PointData,NamedTuple(); DimsVolCross = DimsVolCross, Depth_extent = Depth_extent, DimsSurfCross = DimsSurfCross, section_width = section_width)
+ extract_ProfileData!(Profile, VolData, SurfData, PointData, NamedTuple(); DimsVolCross = DimsVolCross, Depth_extent = Depth_extent, DimsSurfCross = DimsSurfCross, section_width = section_width)
return nothing
end
-
"""
This reads the picked profiles from disk and returns a vector of ProfileData
"""
diff --git a/test/test_ProfileProcessing.jl b/test/test_ProfileProcessing.jl
index 0a27c17..b3d9739 100644
--- a/test/test_ProfileProcessing.jl
+++ b/test/test_ProfileProcessing.jl
@@ -80,9 +80,9 @@ GeophysicalModelGenerator.create_profile_point!(prof4, Data.Point, section_width
@test length(prof1.PointData[1].lon) == 13
@test length(prof4.PointData[1].lon) == 445
-# test screenshot data
+# test screenshot data
GeophysicalModelGenerator.create_profile_screenshot!(prof5, Data.Screenshot)
-@test prof5.SurfData[1].fields.x_profile[1,1,1] == 0
+@test prof5.SurfData[1].fields.x_profile[1, 1, 1] == 0
# Test the main profile extraction routines: |
| @@ -29,6 +29,7 @@ mutable struct ProfileData | |||
| VolData::Union{Nothing, GeophysicalModelGenerator.GeoData} | |||
| SurfData::Union{Nothing, NamedTuple} | |||
| PointData::Union{Nothing, NamedTuple} | |||
| ScreenshotData::Union{Nothing, NamedTuple} | |||
There was a problem hiding this comment.
Why don't you add it as:
ScreenshotData::Union{Nothing, NamedTuple}=nothingor
ScreenshotData::Union{Nothing, NamedTuple}=NamedTuple()In that case, the tests below are not forced to add an empty NamedTuple() if this is not required
There was a problem hiding this comment.
I get the following error when I use the first suggestion:
ERROR: LoadError: syntax: "ScreenshotData::Union{Nothing, NamedTuple} = nothing" inside type definition is reserved around /Users/mthiel/.julia/dev/GeophysicalModelGenerator/src/ProfileProcessing.jl:9
The same error occurs when I use the empty NamedTuple.
There was a problem hiding this comment.
I could also solve this issue via multiple dispatch.
There was a problem hiding this comment.
true, I overlooked the fact that this is a struct definition. You can add a function like:
ProfileData(VolData, SurfData, PointData, ScreenshotData=nothing) = ProfileData(VolData, SurfData, PointData, ScreenshotData)There was a problem hiding this comment.
I added an additional extract_ProfileData! function. Tests now run without including a NamedTuple() argument. Is that what you had in mind?
test/test_ProfileProcessing.jl
Outdated
| extract_ProfileData!(prof2, VolData_combined1, Data.Surface, Data.Point) | ||
| extract_ProfileData!(prof3, VolData_combined1, Data.Surface, Data.Point) | ||
| extract_ProfileData!(prof4, VolData_combined1, Data.Surface, Data.Point) | ||
| extract_ProfileData!(prof1, VolData_combined1, Data.Surface, Data.Point, NamedTuple()) |
|
|
||
| # add the data set as a NamedTuple | ||
| data_NT = NamedTuple{(DataSetName[idata],)}((data_tmp,)) | ||
| tmp = merge(tmp, data_NT) |
There was a problem hiding this comment.
Probably not important, but this is creating a type instability becuse tmp is changing of type every time you add a new field into it.
There was a problem hiding this comment.
Isn't the type defined when tmp is initialized as NamedTuple? If not, would there be any possibility to resolve this? As we use this kind of procedure not only in create_profile_screenshot!, but also in create_profile_surface! and create_profile_point!, we could fix it everywhere.
There was a problem hiding this comment.
The type of a the variables inside the NamedTuple, as well as its length, are in the type signature of the NamedTuple. So even the types of n1 = (;a=1) and n2 = (;a=2, b=1) are different because length(n1)=1 and length(n2)=2. In your code above tmp is initilised as an empty tuple, and then it changes its type when data_NT is merged. This can be seen running the code below.
function foo(a,b,c)
tmp = NamedTuple()
T0 = typeof(tmp)
println("type of tmp: ", typeof(tmp))
tmp = merge(tmp, (;a))
println("type of tmp: ", typeof(tmp))
tmp = merge(tmp, (;b))
println("type of tmp: ", typeof(tmp))
tmp = merge(tmp, (;c))
println("type of tmp: ", typeof(tmp))
Tf = typeof(tmp)
println("T0 == Tf : ", T0 == Tf)
end
a,b,c=1,2,3julia> foo(a,b,c)
type of tmp: @NamedTuple{}
type of tmp: @NamedTuple{a::Int64}
type of tmp: @NamedTuple{a::Int64, b::Int64}
type of tmp: @NamedTuple{a::Int64, b::Int64, c::Int64}
T0 == Tf : falseThere was a problem hiding this comment.
As we do not know the number of fields or datasets beforehand, I don't think we can get around this issue.
…hout an empty Named Tuple for ScreenshotData
To allow screenshots to be saved in ProfileData, I added this field. I also added the respective function create_profile_screenshot! and adapted extract_ProfileData. Tests were also modified.