Write fsgrid domain size as a 64bit number, instead of 32bit.

[ursg]

Without this, writing a 5th refinement level would have silently failed and restarting from those file (or plotting them) would have just given bogus data.

This fixes #517.

Tested with the restart_write and restart_read tests, and by plotting files with visit.

[ykempf]

Is it compatible with older restarts?

…

On 8 February 2021 19:50:20 EET, Urs Ganse ***@***.***> wrote: Without this, writing a 5th refinement level would have silently failed and restarting from those file (or plotting them) would have just given bogus data. This fixes #517. Tested with the restart_write and restart_read tests, and by plotting files with visit. You can view, comment on, or merge this pull request online at: #532 -- Commit Summary -- * Write fsgrid domain size as a 64bit number, instead of 32bit. -- File Changes -- M iowrite.cpp (2) -- Patch Links -- https://github.com/fmihpc/vlasiator/pull/532.patch https://github.com/fmihpc/vlasiator/pull/532.diff -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: #532

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

[ursg]

Yep. It actually only changed a single line, where the data was written.
Indeed, since I just checked: we never even read MESH_DOMAIN_SIZES in the fsgrid reader code, so this would only break the Visit plugin reading.

[ykempf]

So, should this be added to said plugin then?

…

On 8 February 2021 22:26:52 EET, Urs Ganse ***@***.***> wrote: Yep. It actually only changed a single line, where the data was written. Indeed, since I just checked: we never even read MESH_DOMAIN_SIZES in the fsgrid reader code, so this would only break the Visit plugin reading. -- You are receiving this because you commented. Reply to this email directly or view it on GitHub: #532 (comment)

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

[markusbattarbee]

According to #517, the Visit plugin already reads it correctly so no change is required:

In fact, the visit plugin reads this number as a int64_t, and it seems to be by pure coincidence that it works correctly at all: > https://github.com/fmihpc/vlsv/blob/master/visit-plugin/mesh_metadata_visit_ucd_generic_multi.cpp#L357

[markusbattarbee]

Hmm, actually looks like line

vlasiator/iowrite.cpp

Line 583 in 81d92b2

uint32_t domainSize[numberOfDomainTypes];

also has the mesh domain size as uint32_t

And in ioread.cpp we do read the mesh domain size but that's already uint64_t.

[markusbattarbee]

vlsvdiff.cpp line 259
std::array<uint32_t,2> meshDomainSize({globalIds.size(), 0});

[ursg]

Hmm, actually looks like line

vlasiator/iowrite.cpp

Line 583 in 81d92b2

uint32_t domainSize[numberOfDomainTypes];

also has the mesh domain size as uint32_t

True, but in that case I'm not so worried about it, since that affects vlasovGrid and individiual velocity grids only. For those, we are still very comfortably away from 2³² cells. Of course it doesn't really hurt to just change that also, then we should be future-proof forever in this respect.

And in ioread.cpp we do read the mesh domain size but that's already uint64_t.

And those are really just the velocity space mesh domains. They will remain small for all forseeable future. :)

[markusbattarbee]

But does the visit plugin read those in as uint64s? Even if we don't need the memory space quite yet, we should consolidate the variable size between the different access methods.

[ursg]

vlsvdiff.cpp line 259
std::array<uint32_t,2> meshDomainSize({globalIds.size(), 0});

Good catch, I'm fixing and testing this.

[ursg]

But does the visit plugin read those in as uint64s? Even if we don't need the memory space quite yet, we should consolidate the variable size between the different access methods.

Yes it does, see here: https://github.com/fmihpc/vlsv/blob/master/visit-plugin/mesh_metadata_visit_ucd_amr.cpp#L251
The vlsvReader::read function at the moment does the datatype upconversion from 32bit to 64bit here: https://github.com/fmihpc/vlsv/blob/master/vlsv_reader.h#L110

[markusbattarbee]

I think we could move this up to Vlasiator-5.1 as it is required for compiling with AOCC / clang on some systems

[ursg]

And actually: is there anything preventing us from merging this immediately? It seems to work well and improve compilation on AOCC.

[markusbattarbee]

Let it merge!