F2I-Consulting / fesapi

DevKit for ENERGISTICS™ data standards (mainly RESQML™), multi-languages (C++, Java, C#, Python)
Apache License 2.0
32 stars 24 forks source link

GridConnectionSet CellIndexPair null value should allow -1 #293

Closed pmuron closed 3 years ago

pmuron commented 3 years ago

What are the steps to reproduce this issue?

A very common industry practice is to use -1 for undefined index value. we use say (id, -1) to represent "half-connections" to model boundaries or connections to aquifers/thermal boundaries.

What does happen?

Most if the API is uint64_t (unsigned). While we can work with it since uint64_t(-1) is perfectly legit, this is impractical as it cast to very large number and most client just do as (< 0) check -and- we run into issues as uint64_t(-1) is not supported.

Error: The XML null value cannot be greater than a 64 bits signed integer cause of gsoap mappings

What were you expecting to happen?

API is int64_t based for both inputs cell index pairs and null value.

Any logs, error output, etc?

See above when you use uint64_t(-1) as null value: "The XML null value cannot be greater than a 64 bits signed integer cause of gsoap mappings".

Any other comments?

What versions of fesapi are you using?

2x from python bindings right now

philippeVerney commented 3 years ago

I agree. There is indeed a bug there. It looks there is a mix between RESQML2.0.1 where XML schema (gsoap) could allow uint64_t.max as null value and RESQML2.2 where XML schema (gsoap) can no more allow uint64_t.max as null value. The result is something inconsistent in FESAPI method declarations.

I guess the easiest, the most practical and the most standard-compliant regarding newer standard versions, is, as you suggest, to only use int64_t. I'll work on it.

As a side note, be aware that client just doing as (< 0) check do not honor the standard which decided to explicit the null value, even if they follow the common industry practice. But I am pretty sure that you already know that and, furthermore, that's a discussion about the standard itself not really about FESAPI which just honors the standard rules.

As a workaround which is the solution I currently use (which explain why I did not notice this bug), my nullValue is generally set to the max number of items + 1. In this particular case, instead of using -1, I would use cellCount as the nullvalue. and on consumer side we don't rely at all on a -1 check according to the standard but to this nullValue.

Thanks for this valuable issue!

pmuron commented 3 years ago

Awesome

Most of our code bases tend to used SIGNED indices even if UNSIGNED would theoretically make sense and allow broader range of indices (esp. when using only 32-bits).

But negative indices are used and have special meaning either to identify null values or sometimes to pack more information sometimes.

I am not suggesting to part ways from conventions of explicitly defining null value, this is the right thing and we do need it. One code base in using -1 but the other one is using int32.max. So we do need it.

I am just saying we need -1 to be permitted as both null value and as cell pair indices. Hope this makes sense.