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 Gadget2 format #264

Merged
merged 4 commits into from
Mar 2, 2021
Merged

add Gadget2 format #264

merged 4 commits into from
Mar 2, 2021

Conversation

islent
Copy link
Contributor

@islent islent commented Jul 10, 2020

Hello guys,
Gadget2 is a widely used astrophysical simulation code, and it has a user-defined data format. I've implemented I/O interfaces in AstroIO.jl.
Hoping it to be supported via FileIO!
Thanks

Hello guys,
    `Gadget2` is a widely used astrophysical simulation code, and it has a user-defined data format. I've implemented I/O interfaces in [AstroIO.jl](https://github.com/JuliaAstroSim/AstroIO.jl).
    Hoping it to be supported via FileIO!
Thanks
@codecov
Copy link

codecov bot commented Jul 10, 2020

Codecov Report

Merging #264 (bc959e0) into master (7c2c5a2) will increase coverage by 11.84%.
The diff coverage is 68.42%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #264       +/-   ##
===========================================
+ Coverage   73.36%   85.20%   +11.84%     
===========================================
  Files           8        9        +1     
  Lines         443      588      +145     
===========================================
+ Hits          325      501      +176     
+ Misses        118       87       -31     
Impacted Files Coverage Δ
src/error_handling.jl 53.33% <0.00%> (+14.87%) ⬆️
src/registry_setup.jl 95.04% <ø> (+3.04%) ⬆️
src/precompile.jl 73.46% <40.00%> (+73.46%) ⬆️
src/registry.jl 85.60% <45.45%> (-7.05%) ⬇️
src/query.jl 89.56% <77.27%> (+2.55%) ⬆️
src/loadsave.jl 88.54% <83.33%> (+1.19%) ⬆️
src/FileIO.jl 100.00% <0.00%> (ø)
... and 5 more

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 08e348c...7552e7d. Read the comment docs.

@timholy
Copy link
Member

timholy commented Aug 13, 2020

I take it this is different from https://www.gearslutz.com/board/product-alerts-older-than-2-months/1246456-korg-announces-gadget-2-windows-support.html? Do you know what file extensions that other product uses?

Is it true that there are no magic bytes? If you've not read my rant, but possibly have influence over Gadget 2 development, please convey our strong wishes that they add some magic bytes.

@islent
Copy link
Contributor Author

islent commented Aug 13, 2020

Oh, sorry that I did not provide adequate description about Gadget2.

It was developed nearly 20 years ago and no magic bytes used: https://wwwmpa.mpa-garching.mpg.de/gadget/
This software is designed for heavy load particle simulations, and some researches were published on Nature. Its snapshot format is widely used in astronomy community, and supported by many code projects, for example, the yt-project.

Its file structure is:

  1. header struct with fixed length (256 bytes)
  2. data blocks,. Start and end with two identical integers which indicate the length of data block

The developers have migrated the output format to HDF5, but the Gadget format is still supported for old datasets.

If you minds the magic bytes problem, you could simply close this PR. I would just leave format selection problem to users.

@timholy
Copy link
Member

timholy commented Aug 13, 2020

Thanks for the info. Perhaps we could handle the longer extensions since they are less likely to be ambiguous. g2 and G2 seem ambiguous (https://file.org/extension/g2), so I'd propose we delete those from the automatic-loading mechanism.

The other option would be to write a detector; you could, for example, check that the file length is at least 256 bytes and then check whether you get those blocks with identical integer pairs. You can check existing examples in src/registry.jl for specialized format-detectors.

@islent
Copy link
Contributor Author

islent commented Aug 13, 2020

Got it! I would delete g2 and G2, and write a detector just like detecthdf5. This would be done in a few days.

Thanks for your kindly guides :D

@islent
Copy link
Contributor Author

islent commented Sep 27, 2020

I've done some local test. All good. Ready to merge :D

@islent
Copy link
Contributor Author

islent commented Feb 24, 2021

Local test gives:

julia> versioninfo()
Julia Version 1.7.0-DEV.334
Commit 33573eca11 (2021-01-19 07:48 UTC)
Platform Info:
  OS: Windows (x86_64-w64-mingw32)
  CPU: Intel(R) Xeon(R) W-10885M CPU @ 2.40GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-11.0.1 (ORCJIT, skylake)

(@v1.7) pkg> test FileIO
     Testing FileIO
      Status `C:\Users\leois\AppData\Local\Temp\jl_1M1SFi\Project.toml`
  [5789e2e9] FileIO v1.4.5 `E:\JuliaAstroSim\FileIO.jl\`
  [48062228] FilePathsBase v0.9.7
  [44cfe95a] Pkg `@stdlib/Pkg`
  [9a3f8284] Random `@stdlib/Random`
  [8dfed614] Test `@stdlib/Test`
      Status `C:\Users\leois\AppData\Local\Temp\jl_1M1SFi\Manifest.toml`
  [5789e2e9] FileIO v1.4.5 `E:\JuliaAstroSim\FileIO.jl\`
  [48062228] FilePathsBase v0.9.7
  [0dad84c5] ArgTools `@stdlib/ArgTools`
  [56f22d72] Artifacts `@stdlib/Artifacts`
  [2a0f44e3] Base64 `@stdlib/Base64`
  [ade2ca70] Dates `@stdlib/Dates`
  [f43a241f] Downloads `@stdlib/Downloads`
  [b77e0a4c] InteractiveUtils `@stdlib/InteractiveUtils`
  [b27032c2] LibCURL `@stdlib/LibCURL`
  [76f85450] LibGit2 `@stdlib/LibGit2`
  [8f399da3] Libdl `@stdlib/Libdl`
  [56ddb016] Logging `@stdlib/Logging`
  [d6f4376e] Markdown `@stdlib/Markdown`
  [a63ad114] Mmap `@stdlib/Mmap`
  [ca575930] NetworkOptions `@stdlib/NetworkOptions`
  [44cfe95a] Pkg `@stdlib/Pkg`
  [de0858da] Printf `@stdlib/Printf`
  [3fa0cd96] REPL `@stdlib/REPL`
  [9a3f8284] Random `@stdlib/Random`
  [ea8e919c] SHA `@stdlib/SHA`
  [9e88b42a] Serialization `@stdlib/Serialization`
  [6462fe0b] Sockets `@stdlib/Sockets`
  [fa267f1f] TOML `@stdlib/TOML`
  [a4e569a6] Tar `@stdlib/Tar`
  [8dfed614] Test `@stdlib/Test`
  [cf7118a7] UUIDs `@stdlib/UUIDs`
  [4ec0a83e] Unicode `@stdlib/Unicode`
  [deac9b47] LibCURL_jll `@stdlib/LibCURL_jll`
  [29816b5a] LibSSH2_jll `@stdlib/LibSSH2_jll`
  [c8ffd9c3] MbedTLS_jll `@stdlib/MbedTLS_jll`
  [14a3606d] MozillaCACerts_jll `@stdlib/MozillaCACerts_jll`
  [83775a58] Zlib_jll `@stdlib/Zlib_jll`
  [8e850ede] nghttp2_jll `@stdlib/nghttp2_jll`
     Testing Running tests...
There was an error in magick function detect_gadget2
Please open an issue at FileIO.jl. Error:
EOFError()
There was an error in magick function detect_gadget2
Please open an issue at FileIO.jl. Error:
EOFError()
bedGraph: Test Failed at E:\JuliaAstroSim\FileIO.jl\test\query.jl:283
  Expression: FileIO.detect_bedgraph(io)
Stacktrace:
 [1] (::var"#22#31")(io::IOStream)
   @ Main E:\JuliaAstroSim\FileIO.jl\test\query.jl:283
 [2] open(f::var"#22#31", args::String; kwargs::Base.Iterators.Pairs{Union{}, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
   @ Base .\io.jl:330
 [3] open(f::Function, args::String)
   @ Base .\io.jl:328
 [4] macro expansion
   @ E:\JuliaAstroSim\FileIO.jl\test\query.jl:282 [inlined]
 [5] macro expansion
   @ E:\julia\usr\share\julia\stdlib\v1.7\Test\src\Test.jl:1152 [inlined]
 [6] macro expansion
   @ E:\JuliaAstroSim\FileIO.jl\test\query.jl:274 [inlined]
 [7] top-level scope
   @ E:\julia\usr\share\julia\stdlib\v1.7\Test\src\Test.jl:1227
bedGraph: Test Failed at E:\JuliaAstroSim\FileIO.jl\test\query.jl:283
  Expression: FileIO.detect_bedgraph(io)
Stacktrace:
 [1] (::var"#22#31")(io::IOStream)
   @ Main E:\JuliaAstroSim\FileIO.jl\test\query.jl:283
 [2] open(f::var"#22#31", args::String; kwargs::Base.Iterators.Pairs{Union{}, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})        
   @ Base .\io.jl:330
 [3] open(f::Function, args::String)
   @ Base .\io.jl:328
 [4] macro expansion
   @ E:\JuliaAstroSim\FileIO.jl\test\query.jl:282 [inlined]
 [5] macro expansion
   @ E:\julia\usr\share\julia\stdlib\v1.7\Test\src\Test.jl:1152 [inlined]
 [6] macro expansion
   @ E:\JuliaAstroSim\FileIO.jl\test\query.jl:274 [inlined]
 [7] top-level scope
   @ E:\julia\usr\share\julia\stdlib\v1.7\Test\src\Test.jl:1227
┌ Warning: Assignment to `file_dir` in soft scope is ambiguous because a global variable by the same name exists: `file_dir` will be treated as a new local. Disambiguate by using `local file_dir` to suppress this warning or `global file_dir` to assign to the existing global variable.
└ @ E:\JuliaAstroSim\FileIO.jl\test\loadsave.jl:24
┌ Warning: Assignment to `file_path` in soft scope is ambiguous because a global variable by the same name exists: `file_path` will be treated as a new local. Disambiguate by using `local file_path` to suppress this warning or `global file_path` to assign to the existing global variable.
└ @ E:\JuliaAstroSim\FileIO.jl\test\loadsave.jl:25
these tests will print warnings: 
Error encountered while saving "test.not_installed".

Fatal error:
this test will print warnings: 
Errors encountered while loading "C:\\Users\\leois\\AppData\\Local\\Temp\\jl_0jpGg9\\test.multierr".
All errors:
===========================================
1
===========================================
2
===========================================

Fatal error:
Test Summary:                          | Pass  Fail  Total
FileIO                                 |  174     2    176
  OS                                   |    4            4
  DataFormat                           |   16           16
  streams                              |    5            5
  query                                |   23           23
  multiple libs                        |    7            7
  Querying with String                 |   25     1     26
    bedGraph                           |    3     1      4
    STL detection                      |    4            4
    PLY detection                      |    2            2
    Multiple Magic bytes               |    4            4
    AVI Detection                      |    4            4
    RDA detection                      |    4            4
    RDS detection                      |    4            4
  Querying with WindowsPath            |   25     1     26
    bedGraph                           |    3     1      4
    STL detection                      |    4            4
    PLY detection                      |    2            2
    Multiple Magic bytes               |    4            4
    AVI Detection                      |    4            4
    RDA detection                      |    4            4
    RDS detection                      |    4            4
  Format with function for magic bytes |             No tests
  Load String                          |    7            7
  Load WindowsPath                     |    7            7
  Save                                 |   30           30
  Overwrite file with bad magic bytes  |    2            2
  Ambiguous extension                  |    4            4
  Absent file                          |    1            1
  Path errors                          |    3            3
  Not installed                        |    1            1
  Absent implementation                |    2            2
  multiple errors                      |    1            1
  Mime save                            |   10           10
ERROR: LoadError: Some tests did not pass: 174 passed, 2 failed, 0 errored, 0 broken.
in expression starting at E:\JuliaAstroSim\FileIO.jl\test\runtests.jl:13
ERROR: Package FileIO errored during testing
```

Sorry that I cannot understand why there would be `EOFError` in `detect_gadget2`. And what is it about `bedGraph`?

`FileIO` could normally load a `gadget2` file in this case.

Copy link
Member

@timholy timholy left a comment

Choose a reason for hiding this comment

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

The CI logs shouldn't be showing errors for detect_gadget2. These suggested changes (especially the length-check one) may fix it.

I don't get the bedgraph error locally but I'm on Linux. It's not showing up in the CI logs either, so I'm not particularly worried but of course we'd appreciate it if you discover the cause & fix it.

src/registry.jl Outdated Show resolved Hide resolved
src/registry.jl Outdated Show resolved Hide resolved
src/registry.jl Outdated Show resolved Hide resolved
@islent
Copy link
Contributor Author

islent commented Mar 2, 2021

Thanks for helping out!
len > 264 || return false fixes the EOF error.

bedgraph error came up as a result of FileIO.detect_bedgraph returning false. Sorry that I'm not an expert in this format.

Ready to merge :D

@timholy timholy merged commit 07ca373 into JuliaIO:master Mar 2, 2021
@timholy
Copy link
Member

timholy commented Mar 2, 2021

Done, thanks for the contribution!

@islent islent deleted the patch-1 branch March 2, 2021 15:16
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.

None yet

2 participants