scipp / scippneutron

Neutron scattering toolkit built using scipp for Data Reduction. Not facility or instrument specific.
https://scipp.github.io/scippneutron/
BSD 3-Clause "New" or "Revised" License
4 stars 3 forks source link

fix: json nexus loader #502

Closed jokasimr closed 5 months ago

jokasimr commented 5 months ago

Fixes #501

jokasimr commented 5 months ago

I think there might be one thing left to fix here: I added a test for loading the ymir instrument description from gitlab, this is what we will need to load in beamlime (is that right?), but the test does not pass yet. The test was initially marked as skipped because I thought the test failure was related to the other skipped test with the skip message: Stream handling with log not implemented, but I'm not sure that it is.

The test fails with the message:

UserWarning: Failed to load /entry/instrument/laser_monitor as NXmonitor: Signal is not an array-like.

not sure what the issue is there, but it seems like either

Do you have an idea about what the issue might be @SimonHeybrock ?

SimonHeybrock commented 5 months ago

I think there might be one thing left to fix here: I added a test for loading the ymir instrument description from gitlab, this is what we will need to load in beamlime (is that right?),

I think we will mainly be using Loki.

but the test does not pass yet. The test was initially marked as skipped because I thought the test failure was related to the other skipped test with the skip message: Stream handling with log not implemented, but I'm not sure that it is.

The test fails with the message:

UserWarning: Failed to load /entry/instrument/laser_monitor as NXmonitor: Signal is not an array-like.

not sure what the issue is there, but it seems like either

* something about the `laser_monitor` group doesn't conform to our idea of a `NXmonitor`

* or something about the way the structure is handled by the json loader is not correct and that causes the error.

Do you have an idea about what the issue might be @SimonHeybrock ?

This sounds like a problem in either ScippNexus or the file, not the JSON adapter.

jokasimr commented 5 months ago

I think we will mainly be using Loki.

I didn't mean the ymir.json file specifically, just that we will need to read instrument descriptions like those in the gitlab repo. For the record, loading the loki.json file fails with a similar error message:

UserWarning: Failed to load /entry/instrument/loki_detector_0 as NXdetector: Signal is not an array-like.

This sounds like a problem in either ScippNexus or the file, not the JSON adapter.

Okay then I'll mark the test as TODO and leave it as it is right now.

SimonHeybrock commented 5 months ago

I think we will mainly be using Loki.

I didn't mean the ymir.json file specifically, just that we will need to read instrument descriptions like those in the gitlab repo. For the record, loading the loki.json file fails with a similar error message:

UserWarning: Failed to load /entry/instrument/loki_detector_0 as NXdetector: Signal is not an array-like.

This sounds like a problem in either ScippNexus or the file, not the JSON adapter.

Okay then I'll mark the test as TODO and leave it as it is right now.

Actually you simply need to ignore the warning. The loading actually works, it is just a fallback that issues a warning. Since we have strict warning policies (tests fail if there are warnings), you have to allow this warning in this test.

jokasimr commented 5 months ago

Actually you simply need to ignore the warning.

:+1: