Float no longer supports Inf

[greschd]

(decided to open a separate issue instead of discussing on the merged PR)

In #2129, an error is raised when a Float is either nan or inf. The reasoning is that the numeric type of postgres doesn't support these special values. However, as far as I can tell Float is stored as a postgres float, not numeric. I have used Inf in particular extensively without any issues in previous versions.

Is there an error which led to the change in #2129, or was that just cautionary?

[giovannipizzi]

Good question - probably it was just cautionary? @dev-zero do you remember the discussion about this?
Maybe it was also related to hashing?

[greschd]

For what it's worth, hashing still seems to work in my temporary branch where I removed these checks.

[dev-zero]

The problem is that JSON does not allow Inf or NaN in floats. See also the discussion here: https://stackoverflow.com/questions/1423081/json-left-out-infinity-and-nan-json-status-in-ecmascript And since `clean_value` is applied on nested dicts stored as JSON in the DB I made it more restrictive. If this must be supported I'd probably split the function and provide separate implementations for attributes stored directly in the DB as their native datatype (and only use a subset of the checks) and those stored as JSON.

…

On 25 January 2019 09:53:26 CET, Dominik Gresch ***@***.***> wrote: For what it's worth, hashing still seems to work in my temporary branch where I removed these checks. -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #2412 (comment)

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

[greschd]

Ok, I see. If it's just dict (and list?) which have this issue we could just add a flag (like ``restrict_to_json``) which is just set to ``True`` the first time the ``clean_value`` goes through the recursive dict (and list?) comprehension. Or is that too implicit a solution?

[sphuber]

Since this will not be a breaking change as at worst more will be allowed, this is postponed to after v1.0.0

[CasperWA]

I have recently had a problem with this, trying to import StructureData with NaN values.

Looking around, I found a partial solution for the -Inf and Inf values, in the form of using the values -2e308 and 2e308, respectively.
All credit to the ROOT issue here and the related discussion here.
The numbers are legal JSON values, while they are also automatically parsed as -inf and inf in Python.

However, this still leaves an easy fix for NaN.

[greschd]

Looking at this again, I don't completely understand what the issue is with JSON. Does Python maybe add some magic? The following works:

In [1]: import json 
In [2]: json.loads(json.dumps(float('inf')))    
Out[2]: inf

In [3]: json.loads(json.dumps(float('nan'))) 
Out[3]: nan

[greschd]

Ah indeed, the python json module has an allow_nan flag (default: True). By default, it doesn't strictly comply with the JSON standard, and instead adds support for inf and nan: https://docs.python.org/3.5/library/json.html#json.dump

[greschd]

So the question is this: Do we need to be strictly standard-compliant, or is the Python implementation the only one that will ever see our JSONs?

[sphuber]

We don't just rely on being able to dump to file though. Both database backends now use JSONB fields for the attributes and extras. I don't know for sure but maybe Postgres' implementation respects the JSON spec strictly. Try to store and attribute with float('inf') as value

[greschd]

Ah, indeed, that was the reason.

So I guess that leads us back to @dev-zero's suggestion above to split implementations of clean_value between values that will be stored as JSON and those that won't (like for Float).

For #3415 this means we probably won't allow inf and nan in that context, right?

[sphuber]

even the value of Float will be stored in the attributes column and therefore in the JSONB field

[greschd]

Ah, I see.. this is a more recent change, right? As far as I can remember, when we initially discussed this I could "fix" Float by just un-commenting the check in clean_value.

What's the rationale for not storing it as a float field?

[sphuber]

Because all data properties are stored as attributes. It used to be possible because in the old EAV schema (for Django) we did have float fields for float values. However, now the attributes are just stored in a single JSONB field.

[greschd]

Makes sense - that's probably also better for consistency across DB backends. Do you have any other ideas for how to solve this issue? Or should we close it because it's not likely to be implemented any time soon?

[greschd]

We could take inspiration from the python JSON implementation, and just auto-convert inf and nan to strings 'Infinity' and 'NaN' -- that could be done by modifying the value getter and setter in the Float class. Any thoughts on that?

[giovannipizzi]

We can do conversion from those values to floats when storing if we don't care about comparison directly in the DB and type mismatch is not a problem. Probably, could be a solution with #3415 (note however that the query builder comparison would be the one for NaN always returning False, and not the one for infinity, i.e. for instance 3 < 'Infinity' is False.
For this reason, I am not sure this is the best approach in general.

Also, what I would be also in general against is to infer from a string if it used to be a float('inf') (at least in a general way in AiiDA - it's probably ok to do for a specific field, in a data subclass or similar, if you know what data values you expect, even if the problem with comparison remains) - we did it for dates (now dropped) and beside slowing down things, it is not very robust and would lead to unexpected things like you store the string 'Infinity' and when you get it back it's a float('inf').

[greschd]

I'm not sure I completely understand the first point -- is it that the QueryBuilder (any maybe other things) look directly at the attributes, and don't go through the .value property? In that case, I agree that it is problematic.

Regarding the second point, to be clear: This suggestion would be for the Float class only. Since there we know the value attribute should be a float, it would be possible to do this conversion. For generic attributes it would indeed be a bad idea - precisely because you can't know if it's supposed to be the string 'Infinity' or a float that was converted.

[greschd]

This also wouldn't solve #3415, since there it's an attribute of a different class.

[giovannipizzi]

is it that the QueryBuilder (any maybe other things) look directly at the attributes, and don't go through the .value property?

Correct, this is for efficiency reasons (delegating the query to the DB and not fetching all data first in python to run .value and then filtering.

Regarding the second point, to be clear: This suggestion would be for the Float class only.

I see - for the Float class only one might think to do it, but it would make the behavior very different from when putting a float as a value in a list or dict, so I'm not sure it's better to support these values only for single Floats

[greschd]

Correct, this is for efficiency reasons (delegating the query to the DB and not fetching all data first in python to run .value and then filtering.

I think this makes it a bad idea to do some magic in the vaue property. After all, the unprocessed value can leak anyway, which could lead to subtle bugs / inconsistencies.

Unless there are other suggestions, I'd vote to close this issue as "wontfix".

[sphuber]

I agree, so will close it for now

[greschd]

I'm reopening this because there is still a remaining issue: Since previous versions of AiiDA supported NaN / Inf, they can be in existing exports -- these break when trying to import on a new AiiDA instance. I'm also not sure if a migration path exists.

[greschd]

Could also be a separate issue, but I think the discussion here is valuable.

Yeah no, I'll make a separate issue.