marcguetg / h5particle

1 stars 0 forks source link

units simplification #11

Open DavidSagan opened 6 years ago

DavidSagan commented 6 years ago

I propose that the new standard should not differentiate between different kinds of real and integer numbers. Why? First, it makes things simpler. Second, if, for example, someone needed quad precision for some numbers and the standard mandated double precision this is a problem.

One alternative would be to say simply say at the beginning of the standard that all reals should be at least double precision.

Comments?

RemiLehe commented 6 years ago

Thanks for this suggestion. Could you quote the specific part of the standard that you are referring to here? Are you referring to the fact that we impose the precision of the metadata attributes (e.g. timeUnitSI: double). If yes, I guess this makes sense to me, especially as this does not seem to be consistent throughout the document, now that I look at it.

@ax3l Any idea why we specified e.g. double for timeUnitSI and only float for time?

DavidSagan commented 6 years ago

Yes timeUnitSI: double is an example and all the places where uint is mentioned.

ax3l commented 6 years ago

Hi there,

thanks for the suggestions! They should really go into the repo of the standard so we can keep track of them and prioritize what we want next :)

I propose that the new standard should not differentiate between different kinds of real and integer numbers. Why? First, it makes things simpler. Second, if, for example, someone needed quad precision for some numbers and the standard mandated double precision this is a problem.

We do not differenciate wherever it is possible. The only thing that we do enforce is that unit conversion factors, usually just small attributes, need double precision.

Any idea why we specified e.g. double for timeUnitSI and only float for time?

This is just a misunderstanding. The first section of the standard, Conventions Throughout these Documents, defines that a plain (float) means choose any precision:

The naming (float) without a closer specification is used if the implementor can choose which kind of floating point precision shall be used.

Since you are not the first ones confused by that, I would propose to refactor it from (float) to (floatX) so it can not be confused with single precision, floating point numbers :) Same is true for (u)int :) I opened an issue on https://github.com/openPMD/openPMD-standard/issues/146 and will put it in the 1.0.1 revision (and the following release after that) these days if you agree.

DavidSagan commented 6 years ago

@ax3l: Yes it is understood that (float) means any precision. That is not the issue. The issue is why (double) and (uint), etc are used and why not simply just use (float) and (int) and get rid of anything else?

Also there is work on the openPMD 2.0 standard being done on the Wiki page of this repo. Please take a look and tell us your thoughts. One thing that will need to happen is that the issues on the openPMD repo will need to be merged with the new standard.

ax3l commented 6 years ago

Yes it is understood that (float) means any precision. That is not the issue. The issue is why (double) and (uint), etc are used and why not simply just use (float) and (int) and get rid of anything else?

I think I need a more concrete example where this causes an issue for you to understand it better. In most cases, record components (the heavy user data) can have any precision or type a user wants. We are agnostic to that. The only idea to have some conversion attributes fixed in high precision is to make reading of data in strongly typed languages like C/C++ a lot easier.

Also there is work on the openPMD 2.0 standard being done on the Wiki page of this repo. Please take a look and tell us your thoughts. One thing that will need to happen is that the issues on the openPMD repo will need to be merged with the new standard.

We will love to incorporate your proposal step by step, that is amazing work!

Nevertheless, please do not get this wrong but the openPMD project is active, has a clear strategy on how to update formats and invites your contributions in the regular channels. We have no received contributions from you upstream yet and were not contacted privately or publicly if we have not missed anything. As long as we are not able to merge, please avoid a naming collision of the projects. The upcoming changes so far of openPMD 2.0 are documented in https://github.com/openPMD/openPMD-standard/milestone/4 and we can certainly add your contributions in this milestone, an extension or any later version in case we can't find a general solution immediately.

As said in the other issue here: it is also totally fine if you plan to move quick and break things for your specific domain needs. We can also generalize them at any later point into the mainline whenever you want to contribute them. Please just avoid confusion to the community with a proper naming in the meantime :)

DavidSagan commented 6 years ago

@ax3l

The only idea to have some conversion attributes fixed in high precision is to make reading of data in strongly typed languages like C/C++ a lot easier.

I don't understand this. If you only fix the precision for a few quantities how does that make things easier? And why did you choose some but not others? It makes no sense to me that timeUnitSI is mandated double while time is not. Or why are position components "(float) or (int) or (uint)"?

We have no received contributions from you upstream yet and were not contacted privately or publicly if we have not missed anything.

@RemiLehe @jlvay @ChristopherMayes @marcguetg The history of this is that just recently Chris Mayes, Marc Guetg, Andreas Adelmann and I were discussing developing a standard for storing particle data suitable to be used for particle accelerators. We did not know about openPMD until about a month ago when I met Jean-Luc Vay at a workshop. After talking it over, the idea formed to use the openPMD standard as a basis for developing a standard usable for a our needs. There was then a teleconference on October 30 with a bunch of people including people from CAMPA. It looks like you never got on the mailing list and I am sorry for the oversight. After looking at the openPMD standard, my thought was that it would be more efficient to just draft a new standard as a basis for developing things rather than using the issue system since there were a lot of interconnected issues here.

I do hope we can work together on this since having a single standard would be beneficial to everyone and again I am sorry about you not being in the loop.

ax3l commented 6 years ago

I totally agree, thanks for the heads up!

For your specific question about units: position components are record components which are potentially heavy user data. (float) or (int) or (uint) just means a user can put anything, we won't impose any re-formatting or casts on heavy data.

timeUnitSI is a unitSI conversion attribute. It's light, it's a single scalar and purely (in some file formats properly cached) meta data. Since it's doing unit conversion, we imposed double precision. A reader will always just read in exactly a float and does not have to check & re-allocate the given type in advance.

time is currently only a (user) attribute but can be potentially a regular record (heavy data) in future versions, e.g. for time stagged/boosted domains. As such, it's again up to the user what to put there, if they need double or single precision we do not specify. (We could indeed also allow int there and you can already use double for it in 1.0.) For e.g. simulations, most codes would also not read this attribute but would orientate themselves, e.g. for restarts, just on the much easier to fetch iteration (which is not necessarily straight forward to convert to a time).

Since we do not use time much than for post-processing yet, we can absolutely refine it and have a few ideas how: https://github.com/openPMD/openPMD-standard/issues/141 https://github.com/openPMD/openPMD-standard/issues/122

DavidSagan commented 6 years ago

@ax3l

timeUnitSI is a unitSI conversion attribute. It's light, it's a single scalar and purely (in some file formats properly cached) meta data. Since it's doing unit conversion, we imposed double precision. A reader will always just read in exactly a float and does not have to check & re-allocate the given type in advance.

The reading program should have a generic routine to read in integers or reals and convert to a real so I don't see that mandating a float here but not other places saves any work. Plus there is a problem that if someone needed quad precision for this there is a conflict with the standard. So I would say it would be better to not mandate the precision.

ax3l commented 6 years ago

I see your point and can indeed be useful to some. A pragmatic question: will you need quad precision for such meta attributes? (Or only for "heavy data"?) We just saw no demand to have higher precision than double for it yet and double (float64) is available on all common platforms with HW support in case one is really transforming a full nD array on read with it.

At the end, you only need the unitSI attributes for scales on plotting labels and maybe inter-code exchange where other sources of numerical errors are dominating over the 16 significant digits of a double.

ax3l commented 6 years ago

One alternative would be to say simply say at the beginning of the standard that all reals should be at least double precision.

A note on that: that would not be feasible, since we are computing nearly 100% of our simulations in single precision and are even experimenting with half precision.

DavidSagan commented 6 years ago

@ax3l

A pragmatic question: will you need quad precision for such meta attributes?

For my present simulations no but future simulations for the electric dipole moment could very well need quad precision.

A note on that: that would not be feasible, since we are computing nearly 100% of our simulations in single precision and are even experimenting with half precision.

Interesting. Thanks for the info.

ax3l commented 6 years ago

For my present simulations no but future simulations for the electric dipole moment could very well need quad precision.

Ok, good to know. But even in that case I wonder if quad precision for the record components is not what you actually want (which is possible already) and the conversion factor to SI is probably still only used for visualization / post-processing? E.g. during data read/write in the same simulation one usually never accesses that attribute since one stays in the same unit system.

It also depends if your workflows need this attribute on read at all: if you are internally computing in SI than we usually skip the multiply with a unit in readers if the unitSI is found to be exactly 1.0.

DavidSagan commented 6 years ago

@ax3l

I guess the point here is that mandating the precision and mandating things like "uint64" unnecessarily complicates the standard. I think that for simplicities sake the standard should be units agnostic unless there was a concrete reason not to. Can you come up with a realistic scenario where not specifying precision/uint would lead to trouble? You mention that casting might complicate reading a data file but has this ever been true in practice?

ax3l commented 6 years ago

I understand that point and it is valid indeed.

Our progression model is just a bit the other way around: since we are actively building tools and libraries around the standard in various languages, we decided that not having to check the input type of basic attributes will just make reading way more easier to start with. Just think about the C/Fortran HDF5 API for reads as an example, it's a pain to read a simple attribute that one does not know what type it has. Switch cascades, dynamic allocations and typecasts everywhere.

That does not mean we are not planning to relax constrains in the future when needed, but just having to expect any type for attributes adds a whole lot of checking, type casts and error potential that we did not want to introduce for a first version and we should motivate when this pain is needed the moment we introduce it. It's not about writing, it's about reading the data.

DavidSagan commented 6 years ago

@ax3l

it's a pain to read a simple attribute that one does not know what type it has. Switch cascades, dynamic allocations and typecasts everywhere.

I don't understand this. First of all, Just because that type is not specified in the standard does not mean that in a given file the type is unknown. And the way to handle, say, different types of reals is to have a read module that handles all the different real types. There are not that many cases to deal with and the module will be fairly compact. Since this one module can be used for all real attributes the amount of code and hence the error potential is minimal. and the read real module simply returns the attribute value in the type that is suitable for your program (say type double) so you don't need type casts or dynamic allocations for this.

Some pseudo code might look like this:

double read_real(attribute_name)
  attribute_type = hdf5_get_attribute_type(attribute_name)
  if (attribute_type == hdf5_double) {
    double dvalue
    status = hdf5_read_double(attribute_name, dvalue)
    return dvalue
  } elseif (attribute_type == hdf5_real) {
    real rvalue
    status = hdf5_read_double(attribute_name, rvalue)
    return rvalue
  } ...
}

Compared to everything else this amount of code to read all real attributes is trivial.

Can you send me example code for reading an attribute that shows the problems you are having?

ax3l commented 6 years ago

Unfortunately it's not as easy as that since the return value needs to be defined outside of the scopes. That means we are fiddling with void* pointers here if we assume e.g. C. the only solution to that is then an immediate type cast. Also, one can not overload on different return types in case on wants to wrap it.

We are well aware how such an API can be written and have done it before, e.g. see https://github.com/ComputationalRadiationPhysics/libSplash/pull/248

It's just way more to do than reading (and possibly asserting) an expected type which is a simple one-liner even if someone programs directly against e.g. HDF5.

Anyway, it's more of a burden on the reader side for the few very basic attributes (not data sets) that actually have a fixed IEEE type and we would need to show a use case where it is needed.

DavidSagan commented 6 years ago

@ax3l

Unfortunately it's not as easy as that since the return value needs to be defined outside of the scopes.

Interesting. Why not always just return a double here?

ax3l commented 6 years ago

Why not always just return a double here?

If the input was of higher precision: one looses precision. If one wants to keep the precision: switch cascade in user code about the choice of the reader function.

If the input was of lower precision: all follow-up operations would be casted up to a significantly (2-3x) more expensive operation (e.g. multiply of a heavy-data record) or one would cast immediately after yet again. Also, one would need to add a runtime check if the input one did read was of the expected precision.

all reals should be at least double precision.

One can definitely do this, but I think it's pragmatically not needed above double for a conversion from "data/simulation scale" to "absolute SI scale for a plot/other sim's input" compared to the other errors one is facing in such a step (color depth of monitors / prints or numerical methods are for 99% of the apps way above that). The data/simulation scale can of course use be of quad precision or any higher (lower) precision already.

if, for example, someone needed quad precision for some numbers and the standard mandated double precision this is a problem.

Please be aware that unitSI-like attributes, aka "absolute SI scale for a plot/other sim's input", are the only points in the standard where we say we want double precision, anything else is up to the user. unitSI is not heavy-user data and does not share the precision of the heavy-user data. openPMD records are heavy-user data, like your electric dipole moment, and can be any precision in openPMD.

at least double precision

Side-note: quad precision (IEEE-754 float128) is not defined on many popular HPC hardware, often lacking (adios) or buggy (e.g. google recent h5py bugs, default visualizer tools of HDF5, ...) in file formats, not defined in or not defined in quite a lot of compilers (e.g. nvcc) and has virtually no known platform with (SIMD) hardware acceleration; making it a bad alternative for the unitSI conversion attribute besides the reasoning above for what it is actually used for in reality (absolute scale of a colorbar). One would need a strong use case to force all data-readers to check for that possibility.

DavidSagan commented 6 years ago

@ax3l

I'm afraid that I did not express myself well so let me try to restate things this way: A given program will be using some precision for its calculations. Say, single precision. If the program reads in an openPMD file it can have the read_real routine (see above) return single precision numbers so there does not have to be any casts and there is no need for the openPMD standard to mandate the precision.