Fix the nx_char type for numpy to and .

[RubelMozumder]

@lukaspie, you can also check this issue with here.

Fix NX data type validation errors:

1. Issue1: The Data type of an element of a list or array is not being checked against the intended data type, e.g., NX_INT. (Status: Fixed)
2. Issue2: If possible, provided data is converted to the intended data type (e.g. "3" -> 3) and no warning is raised. (Status: Fixed, note: boolean conversion is allowed, e.g. true->True).

[lukaspie]

What is this solving? The linked issue is about integers, this is about strings/bytes.

Why don't we allow chararrays anymore?

[rettigl]

Please see #555 and the discussed way for solving this. We should remove arrays generally, and check the array dtype instead.

[RubelMozumder]

What is this solving? The linked issue is about integers, this is about strings/bytes.

Why don't we allow chararrays anymore?

chararry is a data structure like ndarray which stores data according to the premitive datatype. With the current implementation, we are not checking any data structure like list or ndarray but rather the primitive types of that ndarray such as int, bytes_, float, str_, bool.

[RubelMozumder]

@GinzburgLev

Can you remember which issue was handled in the modification https://github.com/FAIRmat-NFDI/pynxtools/blame/nx_char_type/src/pynxtools/dataconverter/helpers.py#L729 ?
Currently, this code converts an integer to a sting and skips the validation warning.

Then I can also test my code for that issue as well.

[GinzburgLev]

@GinzburgLev

Can you remember which issue was handled in the modification https://github.com/FAIRmat-NFDI/pynxtools/blame/nx_char_type/src/pynxtools/dataconverter/helpers.py#L729 ? Currently, this code converts an integer to a sting and skips the validation warning.

Then I can also test my code for that issue as well.

This change was addressing the following issue: #393

The example from this issue should do: nexus file with one of the datasets with type NX_FLOAT, filled with integer = 0 (such as mpes example upload file, with sp.binned.attrs["metadata"]["energy_calibration"]["tof"] = 0 instead of 0.0). The is_valid_data_field() function in helpers.py successfully converted it to float(0.0), but this result was later interpreted as False (as in, failure to convert) by whatever was calling is_valid_data_field(). I can send you the files I used for testing (to big to be attached here).

[RubelMozumder]

@GinzburgLev
Can you remember which issue was handled in the modification https://github.com/FAIRmat-NFDI/pynxtools/blame/nx_char_type/src/pynxtools/dataconverter/helpers.py#L729 ? Currently, this code converts an integer to a sting and skips the validation warning.
Then I can also test my code for that issue as well.

This change was addressing the following issue: #393

The example from this issue should do: nexus file with one of the datasets with type NX_FLOAT, filled with integer = 0 (such as mpes example upload file, with sp.binned.attrs["metadata"]["energy_calibration"]["tof"] = 0 instead of 0.0). The is_valid_data_field() function in helpers.py successfully converted it to float(0.0), but this result was later interpreted as False (as in, failure to convert) by whatever was calling is_valid_data_field(). I can send you the files I used for testing (to big to be attached here).

Thanks, I see it is not from your code. Can you help me to reproduce this error?

[GinzburgLev]

Then I can also test my code for that issue as well.

This change was addressing the following issue: #393
The example from this issue should do: nexus file with one of the datasets with type NX_FLOAT, filled with integer = 0 (such as mpes example upload file, with sp.binned.attrs["metadata"]["energy_calibration"]["tof"] = 0 instead of 0.0). The is_valid_data_field() function in helpers.py successfully converted it to float(0.0), but this result was later interpreted as False (as in, failure to convert) by whatever was calling is_valid_data_field(). I can send you the files I used for testing (to big to be attached here).

Thanks, I see it is not from your code. Can you help me to reproduce this error?

In the version of pynxtools that we had back then, I used dataconverter in the following way:

dataconverter --reader mpes --nxdl NXmpes raw_files/mZlhpEzOQY6Ms4hJbIIu3g/config_file.json raw_files/mZlhpEzOQY6Ms4hJbIIu3g/MoTe2_float.h5

dataconverter --reader mpes --nxdl NXmpes raw_files/mZlhpEzOQY6Ms4hJbIIu3g/config_file.json raw_files/mZlhpEzOQY6Ms4hJbIIu3g/MoTe2_int.h5

The first one worked fine, no errors; the second gave something like "Field /ENTRY[entry]/PROCESS_MPES[process]/energy_calibration/original_axis written without documentation." (aside from usual messages). The resulting file was missing corresponding field (or its value, I am not sure anymore) (output.nxs/entry/process/energy_calibration/original_axis)

The issue was present before this PR was merged:
#522

[RubelMozumder]

Currently, verification of NX data type does not convert the data to other data type anymore rather pop up an warning for any data type inconsistency. For example data 2.0 for NX_INT would not convert to 2 and vice-versa. In NeXus We have specific, data type NX_INT and NX_FLOAT in this case. If 2.0 and 2 is equivalent one can use NX_NUMBER in this case.

Such conversion creates issues in some cases such as for numbers >1 to true and any string e.g. "2" to int/float 2/2.0 and silently validator pass them without any warning massages. Such issues is also observed in here #555. In that issue, a vector attributes has been defined in app def NX_CHAR which is incorrect. As the validator does not create any warning message here, nobody can detect that error that comes through the app def.

[RubelMozumder]

• Nyaml needs to be fixed and merged before merging this PR, so that we can skip the circumvent the errors.

[lukaspie]

LGTM, thanks for the fix. Left some small comments.

Let's wait with the merge until we have the correct definitions here and the plugin tests pass.

[lukaspie]

Suggested change

	# Not to be confused with `np.byte` and `np.ubyte`, these store
	# and integer of `8bit` and `unsigned 8bit` respectively.
	# Not to be confused with `np.byte` and `np.ubyte`, these store
	# integers of `8bit` and `unsigned 8bit` respectively.

What exactly is not to be confused? Maybe write "np.xxx is not to be confused..."

[lukaspie]

Suggested change

	This function also converts bool value comes in str format. In case, it fails to
	convert, it raises an Exception.
	This function also converts boolean value that are given as strings (i.e., "True" to True).

It doesn't really raise an Exception, but just a ValidationProblem.InvalidDatetime warning. What were you trying to say here?

[RubelMozumder]

I do not see the InvalidDateTime warning anywhere.

[rettigl]

Add typing annotation. The calling function expects a single bool.

[rettigl]

The return handling is correct, nevertheless I suggest to add typing everywhere.

[RubelMozumder]

Doc is updated. You might be talking about the return is_valid_data_field(mapping[key], node.dtype, key)[0] in `validation.py. It returns a single first value. Extra variable is added.

[rettigl]

@RubelMozumder @sherjeelshabih I suggest to meet and discuss how to handle this PR and #557 . It does not make sense to independently develop different solutions for the same problem.

[sherjeelshabih]

Agreed. I already plan to merge what Rubel has here with some changes I want to keep in the other branch.

[lukaspie]

From the plugin tests, it does seem that the functionality is working as intended now. We get some easy-to-solve tests for some of the plugins, but we also get warnings like this (this is in pynxtools-mpes):

WARNING  pynxtools:helpers.py:98 WARNING: The value at /ENTRY[entry]/data/AXISNAME_indices[@energy_indices] should be one of the following Python types: (<class 'str'>, <class 'numpy.str_'>, <class 'numpy.bytes_'>, <class 'numpy.chararray'>), as defined in the NXDL as NX_CHAR.

This is an underlying problem with the definitions that we can only fix by using the latest definitions from the fairmat branch. However, this depends on #578 working, so that may need to be merged before this PR.

[rettigl]

This is an underlying problem with the definitions that we can only fix by using the latest definitions from the fairmat branch. However, this depends on #578 working, so that may need to be merged before this PR.

That's a fair point. An alternative to disentangle all these tasks a bit would be to add certain warnings to be expected temporarily, or live with some failing tests for a while, if we know that they should fail.

[rettigl]

Tests fail because it checks out the wrong definitions:


@lukaspie any idea why?

[rettigl]

Closed in favor of #585