Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable nxspe reader for instrument data #1765

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

cmarooney-stfc
Copy link
Collaborator

Draft PR (still debug info in some source) to test reading of these new files with the existing nxspe reader. the changes test the ability to read.

The code to populate instruments with the additional information has not yet been added

This supersedes PR #1714 which is corrupt because it still contains references to the too-large original nxspe test files. The ones here have had their energy bins reduced in size from 640 to 10 to fit within the github limit. The python script used to shrink the files has also been added

Copy link
Member

@abuts abuts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment:
I do not understand the meaning of these changes.
Either you want to read whole instrument from nxspe -- then read_instrument method should be present on nxspe loader, and a dumb read_instrument returning empty instrument on other loaders. and read_instrument shoild return whole insturument.

Or you may want to read part of the instrument -- then bunch of the methods for components should be present with dumb equivalents on the interface.

Here we have some unclear mixture which reads instrument but uses other fake "read" methods to construct instrument. The purpose of such implementation is absolutely unclear to me and IMHO, highly confusing.

Comment on lines +1 to +4
# Script to reduce the number of energy bins in a nxspe file so its
# size is small enough to be uploaded to github as test data

# this script has not been set up to run from the command line - input
Copy link
Member

@abuts abuts Oct 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment:
Adding multiple binary files to repository is very bad practice.
As far as I understand, all these files differ by instrument only and you test generation/saving the instrument and then recovering them.

Why not to generate majority of these files on the fly, save it and test loading and add one small sample file with one sample instrument to validate code consistency? Or even test generation against test with save and autogenerate all other files and test their loading. You are testing these things anyway.

Comment on lines 304 to +306
function moderator = read_inst_moderator_(obj, ds)
% Construct an IX_moderator from a NeXus data structure
moderator_described = true;
Copy link
Member

@abuts abuts Oct 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment:

Hate discussing naming conventions, but why this read does not read anything? It should open appropriate hdf group, throw if not found and call Moderator constructor if found. (or init moderator method). Alternatively, it should return empty moderator||instrument||whatever

as far as I understand, you are reading nxspe files, and nxspe instrument is a structure, generated by other application. binary-serializable should not be used to save nxspe instument. In nxspe, you should be saving a structure similar to the structure, obtained from MANTID. Then recover this structure as instrument. Optianally, you may want to recover instrument components.

Here you should read a structure or partial structure, and place the task of converting the structure into meaninful classes onto class constructor or init method. Why to invent some intermediate workflow?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants