-
Notifications
You must be signed in to change notification settings - Fork 28
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
move fld_read fld_write to FileIO (deprecating) #94
Conversation
src/io/fld-read.jl
Outdated
fld_header() | ||
|
||
Deprecated: use `AVSfldIO.fld_header` instead. | ||
""" | ||
`header = string_to_array(header_lines)` | ||
|
||
convert long string with embedded newlines into string array | ||
""" | ||
function string_to_array(header_lines::String) | ||
newline = '\n' | ||
|
||
# ensure there is a newline at end, since dumb editors can forget... | ||
if header_lines[end] != newline | ||
header_lines *= newline | ||
end | ||
|
||
ends = findall(isequal(newline), header_lines) | ||
(length(ends) <= 0) && throw("no newlines?") | ||
|
||
header = split(header_lines, newline, keepempty=false) | ||
|
||
# strip comments (lines that begin with #) | ||
header = filter(line -> line[1] != '#', header) | ||
|
||
return header | ||
function fld_header() | ||
throw("fld_header is deprecated: use AVSfldIO.fld_header instead.") | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a @deprecate
macro that you can use to handle such code movement without breaking people's codes. Similar suggestions to others.
fld_header() | |
Deprecated: use `AVSfldIO.fld_header` instead. | |
""" | |
`header = string_to_array(header_lines)` | |
convert long string with embedded newlines into string array | |
""" | |
function string_to_array(header_lines::String) | |
newline = '\n' | |
# ensure there is a newline at end, since dumb editors can forget... | |
if header_lines[end] != newline | |
header_lines *= newline | |
end | |
ends = findall(isequal(newline), header_lines) | |
(length(ends) <= 0) && throw("no newlines?") | |
header = split(header_lines, newline, keepempty=false) | |
# strip comments (lines that begin with #) | |
header = filter(line -> line[1] != '#', header) | |
return header | |
function fld_header() | |
throw("fld_header is deprecated: use AVSfldIO.fld_header instead.") | |
end | |
@deprecate fld_header AVSfldIO.fld_header |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks very much for this suggestion. I didn't know about this command!
I will use it in the next commit (before merging this PR).
It's a bit sad though that by default there is no warning given to users:
https://docs.julialang.org/en/v1/base/base/#Base.@deprecate
So it seems like most users will not know about this change until we really remove it anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it seems like most users will not know about this change until we really remove it anyway.
Correct... This is because Julia is designed for high performance, and warnings will hurt the performance. This is another reason that people need to be careful about the semantics versioning 😄
Codecov Report
@@ Coverage Diff @@
## master #94 +/- ##
==========================================
- Coverage 99.79% 99.73% -0.06%
==========================================
Files 49 46 -3
Lines 2406 2251 -155
==========================================
- Hits 2401 2245 -156
- Misses 5 6 +1
Continue to review full report at Codecov.
|
test/io/fld-read.jl
Outdated
using Test: @testset, @test_throws | ||
|
||
|
||
dir = mktempdir() | ||
|
||
|
||
@testset "fld eof" begin | ||
file = joinpath(dir, "fld-read-eof-test.fld") | ||
open(file, "w") do fid | ||
# @show fid | ||
println(fid, "# AVS field file eof test") | ||
end | ||
@test_throws String fld_read(file) | ||
|
||
open(file, "r") do fid | ||
@test_throws String fld_read_single( | ||
file, fid, | ||
(99,), Float32, fieldtype, | ||
false, # is_external_file, | ||
"", # extfile, | ||
Float32, "ieee-le", 0, 0, | ||
) | ||
end | ||
end | ||
|
||
|
||
@testset "fld misc" begin | ||
@test_throws String arg_get(["1", "2"], "3") | ||
@test string_to_array("0") isa AbstractArray{<:AbstractString} | ||
end | ||
|
||
|
||
@testset "fld ascii" begin | ||
tdir = mktempdir() | ||
file = joinpath(tdir, "fld-read-test.fld") | ||
extfilename = "fld-read-test.txt" | ||
chat = isinteractive() | ||
|
||
data = Float32.(1:5) | ||
head = [ | ||
"# AVS field file" | ||
"ndim=1" | ||
"dim1=5" | ||
"nspace=1" | ||
"veclen=1" | ||
"data=float" | ||
"field=uniform" | ||
"variable 1 file=$extfilename filetype=ascii" | ||
] | ||
open(file, "w") do fid | ||
for line in head | ||
println(fid, line) | ||
end | ||
end | ||
|
||
@test fld_header(file) isa Array{<:AbstractString} | ||
|
||
# ensure that it throws if no extfile | ||
@test_throws String tmp = fld_read(file) | ||
|
||
# write ascii and test | ||
extfile = joinpath(tdir, extfilename) | ||
open(extfile, "w") do fid | ||
for d in data | ||
println(fid, d) | ||
end | ||
end | ||
|
||
# run(`op range $file`) | ||
tmp = fld_read(file ; chat=chat) | ||
@test tmp == data | ||
@test eltype(tmp) == Float32 | ||
|
||
rm(file) | ||
rm(extfile) | ||
@testset "fld-read" begin | ||
@test_throws String fld_header() | ||
@test_throws String fld_read() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to delete these tests very quickly. Instead, you can move them to a separate file test/deprecated.jl
so as to make sure old usages are still valid. And then in the future appropriately remove them when the symbols are removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. I'll put a simple and quick version of the test here.
I'd like CI to run faster here, so I'll leave the full tests to AVSfldIO.jl
As I've commented, with |
Although one of my labmates does MRI-related things and once tried this package, I haven't actually had a chance to use this package. But by all means, just feel free to reach for help if there are any similar code maintenance issues and I'd be very much happy to help and review. I wish we could have more good quality image processing toolboxes in Julia and so that more people come using it. |
I have other changes to make soon, so no version bump yet. |
@johnnychen94 |
Can you provide an example of this or maybe submit a draft PR? Without any signature to the methods, it's just a plain symbol deprecation and would apply to all methods if I understand it correctly. julia> f(x, y=1) = x + y
f (generic function with 2 methods)
julia> @deprecate g f
g (generic function with 1 method)
julia> g(1)
┌ Warning: `g` is deprecated, use `f` instead.
│ caller = top-level scope at REPL[4]:1
└ @ Core REPL[4]:1
2
julia> g(1, 2)
┌ Warning: `g` is deprecated, use `f` instead.
│ caller = top-level scope at REPL[5]:1
└ @ Core REPL[5]:1
3 If only a specific method/signature needs to be deprecated, then we need to be careful about the signature: julia> f(x, y=1) = x + y
f (generic function with 2 methods)
julia> @deprecate g(x) f(x)
g (generic function with 1 method)
julia> g(1)
┌ Warning: `g(x)` is deprecated, use `f(x)` instead.
│ caller = top-level scope at REPL[3]:1
└ @ Core REPL[3]:1
2
julia> g(1, 2)
ERROR: MethodError: no method matching g(::Int64, ::Int64)
Closest candidates are:
g(::Any) at deprecated.jl:70
Stacktrace:
[1] top-level scope
@ REPL[4]:1 |
Thanks. The issue that was unexpected to me is that f(x ; y=5) = x + y
@deprecate g f
@show g(1) # works
@show g(2 ; y=3) # fails Oh well. I've addressed it in #96 hopefully. |
Oops! I didn't know that. Looks like this works: @deprecate g(args...; kwargs...) f(args...; kwargs...) |
Perfect! That is the elegant solution I was hoping to use! |
This (not-quite) breaking change moves
fld_read
andfld_write
to the newly improvedFileIO
package that in turn usesAVSfldIO
under its hood. For now these are still supported via@deprecated
.Now one can simply use
load
andsave
fromFileIO
to read/write AVS .fld files.