-
Notifications
You must be signed in to change notification settings - Fork 16
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
1620 Compare Module and Data Source keyword #1646
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may not be fully awake while going through this, but it feels kinda overkill for what we want to do, and I fear is dragging us away from just getting a working 1D comparison up and running correctly, with multiple datasets, which is what we really need.
Perhaps I'm mistaking your intent, but at present this feels like its written to be a catch-all class that can store multiple different dimensionalities of data (which we don't want) rather than something more akin to a templated class taking, for instance, the type and file format (Data1D
and Data1DImportFileFormat
) and which then has specialisations to handle the difficult parts that don't translate across dimensionalities (e.g. the fact that Data1D
derives from a base class, and which complicates the search for internal data of that type).
@trisyoungs The intent is to have a The specialisation (Data1D requiring a different search function) has to be handled in Compare.cpp, as it requires the |
@rhinoella Because of the need to have the processing module data to search? Couldn't you just pass this in as a const reference when creating the keyword? |
@trisyoungs Unfortunately the |
@rhinoella True true. Alas, module constructors used to get passed the main |
Co-authored-by: Tristan Youngs <tristan.youngs@stfc.ac.uk>
Changes
NoteDue to the removal of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks really good. I've just included one small change to fix the Windows build.
Co-authored-by: Adam Washington <adam.washington@stfc.ac.uk>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking really very nice indeed. I have a bunch of formatting comments, but nothing more than that.
Firnat & comment changes Co-authored-by: Tristan Youngs <tristan.youngs@stfc.ac.uk>
Co-authored-by: Tristan Youngs <tristan.youngs@stfc.ac.uk> Co-authored-by: Adam Washington <adam.washington@stfc.ac.uk>
Module to compare two datasets and a new keyword to manage data sources
Compare Module
Data Source Keyword
Notes
Closes #1620