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

[registry] AVS .fld data files #333

Merged
merged 3 commits into from
May 15, 2021
Merged

[registry] AVS .fld data files #333

merged 3 commits into from
May 15, 2021

Conversation

JeffFessler
Copy link
Member

@JeffFessler JeffFessler commented May 13, 2021

This PR adds support for AVS .fld data files, using the new library
https://github.com/JeffFessler/AVSfldIO.jl
(File format etc. is documented there with links to many software frameworks that support it.)
The library is in the middle of the 3-day waiting period for the registry.
I have 100% code coverage in the library but I'm unsure if I need to add any tests to FileIO.jl.
Update: I added magic bytes.

@JeffFessler
Copy link
Member Author

BTW, I'd be happy to have AVSfldIO.jl housed under JuliaIO. Please advise.

@johnnychen94
Copy link
Member

johnnychen94 commented May 13, 2021

Is 1.8.3 the right version or is it 1.9.0 now?

Yes, it should be v1.9.0 since it's a new feature. But given that there're also other new formats registered as pending PRs, it's better to modify the version number outside of these PRs as an independent commit. Can you revert the version change in Project.toml?

I have 100% code coverage in the library but I'm unsure if I need to add any tests to FileIO.jl.

Adding minimal tests to FileIO could help make sure the future development of FileIO does not break the normal functionality of fld data read/write.

BTW, 100% coverage is usually a "fake" number as the coverage test only count if this line gets called:

  • It does not show if different (valid or invalid) arguments (positional and keyword) combinations are handled correctly.
  • it does not check if the other side of the if-branch will be called. For example, !isnothing(x) ? true : some_buggy_code() will still be marked as covered even though some_buggy_code is not tested (due to incomplete test cases or is just a dead code that can never be called.)

There are no magic bytes for this file format.
But I could write a simple detector if it is required for merging.

Yes, I think a header check is needed as a detector. See comments in : #222 (comment)

BTW, I'd be happy to have AVSfldIO.jl housed under JuliaIO. Please advise.

ping @IanButterworth, do we need to invite @JeffFessler as a member first?

@codecov
Copy link

codecov bot commented May 13, 2021

Codecov Report

Merging #333 (1317dd7) into master (f2d16a5) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #333   +/-   ##
=======================================
  Coverage   88.01%   88.01%           
=======================================
  Files          10       10           
  Lines         626      626           
=======================================
  Hits          551      551           
  Misses         75       75           
Impacted Files Coverage Δ
src/registry.jl 90.54% <ø> (ø)

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 f2d16a5...1317dd7. Read the comment docs.

@johnnychen94 johnnychen94 changed the title AVS .fld data files [registry] AVS .fld data files May 13, 2021
@johnnychen94 johnnychen94 mentioned this pull request May 13, 2021
5 tasks
@JeffFessler
Copy link
Member Author

@johnnychen94 I realized that actually this format does have magic bytes, namely "# AVS" so I have updated the PR to include that. So now I think that no further header check is needed.
I also reverted the version as requested.
Please advise what type of test to add and where to put it. I find it hard to follow the testing code in test/ here. Probably loadsave.jl is the file to edit, but it is 400 lines with no obvious place for adding new formats. (Perhaps a separate file of test code for each new type would be more clear?)

@johnnychen94
Copy link
Member

johnnychen94 commented May 14, 2021

Please advise what type of test to add and where to put it. I find it hard to follow the testing code in test/ here. Probably loadsave.jl is the file to edit, but it is 400 lines with no obvious place for adding new formats. (Perhaps a separate file of test code for each new type would be more clear?)

A good question... The current situation from my understanding is that some add tests and some don't. I prefer to have tests because it makes things more reliable.

People add query tests here in test/query.jl so as to make sure FileIO correctly calls one of the available backends. You can take a look at #323 as an example.

@@ -0,0 +1,9 @@
# AVS field file (ascii variant)
Copy link
Member

Choose a reason for hiding this comment

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

Just a question. Does all AVS field file has this # AVS?

Copy link
Member Author

Choose a reason for hiding this comment

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

Every one that I have seen. This format was never blessed by any standards organization and I have been unable to find online documentation of it. (I learned the format from a paper manual before the WWW existed!) So I think we will not find any 100% definitive answer to the question.
I already added a comment about this assumption in the long README here:
https://github.com/JeffFessler/AVSfldIO.jl
That README is probably more complete documentation than you will find anywhere on the web!

It is trivial to add that header if someone (1) has a file without it and (2) wants to read it with Julia.
If at some later point there is someone who has a big pile of the data format without that header then I am happy to write a fancier format detector function if needed.

I have added the query test in the most recent commit.
I also changed the format name from "FLD" to "AVSfld" to be more specific.

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