JuliaAstroSim / AstroIO.jl

I/O interface for astrophysical simulation codes
GNU General Public License v3.0
5 stars 1 forks source link

Fix Gadget2 read and units #8

Closed elehcim closed 3 years ago

elehcim commented 3 years ago

I added a set of units (the default for Gadget2) which is used when reading/writing files. I corrected the units of Potential (linked to https://github.com/JuliaAstroSim/PhysicalParticles.jl/pull/18 ).

Also I fixed the usage of StructArray and added some tests.

I'm available this period to contribute on this.

I have a couple of ideas, don't know if you have already thought about that. For example I'd like to define a getindex method for retrieving only a certain collection from a StructArray of Stars. I'm not super expert of Julia, but I really want to learn more about it. Also, there is the need to add Temperature and Energy and star-particle related fields, e.g.: age, chemical composition... Before going forward, let me know if you have any thought about this. I'll probably open some issues to discuss.

islent commented 3 years ago

The error comes from julia 1.3.0 not supporting keyword abbreviation. Ready to merge.

elehcim commented 3 years ago

ok, thanks. I didn't finish to address the units comments. I see the point of them being configurable, I support that idea in general. In in the manual, in fact, the internal code units are the ones defined in uGadget2. So I'd expect the files to have those units. Maybe for the moment we can leave them like this.

As a side note: I see these days more and more use of hdf5 as output format for Gadget2 (which is indeed more flexible). This file has units embedded in its header, so configurable units are needed in a while. This is for the (far) future (for now I don't even have example files).

I'm planning to create a Gadget2Particle type inside AstroIO to contain the fields needed by Gadget2 files. In this way, PhysicalParticles.jl and AstroIO.jl are a bit less interdependent (e.g. a fact that is causing the CI to fail at the moment because the package PhysicalParticles.jl used now in the tests is a bit outdated).

islent commented 3 years ago

I'm planning to create a Gadget2Particle type inside AstroIO to contain the fields needed by Gadget2 files.

This is the reason that I'm using Star or AbstractParticle type while implementing gravity solvers. Other users may prefer particle types like Electron, Photon, etc. Let abstract types to guarantee the most possible reusability.

a fact that is causing the CI to fail at the moment because the package PhysicalParticles.jl used now in the tests is a bit outdated

Yes, local tests succeeded. A little wierd to me though.

islent commented 3 years ago

For reference, these are critical fields for Gadget-like gravity solver:

    Pos::PVector2D{P}
    Vel::PVector2D{V}
    Acc::PVector2D{A}
    Mass::M
    ID::I
    Collection::Collection

    Ti_endstep::I  # Next integer step on the timeline.
    Ti_begstep::I  # Present integer step on the timeline.
    GravCost::I    # For each two-particle interaction, GravCost += 1

    Potential::E   # Particle potential in the force field
    OldAcc::A      # Save the normalization of acceleration of last step. Useful in Tree n-body method.