Change integer type of Mantid:Nexus::NXInfo #38805

peterfpeterson · 2025-02-07T19:21:51Z

Since nexus actually works with int64_t, change the integer type in NexusClasses.h's NXInfo to match the representation.

This is intended to support #38791 and isolate potential issues because it is a much more substantial change.

Refs #38332

To test:

This changes an internal type from int to int64_t and existing tests will pick up any issues caused by it.

This does not require release notes because it is a code refactor with no user facing changes.

Reviewer

Please comment on the points listed below (full description).
Your comments will be used as part of the gatekeeper process, so please comment clearly on what you have checked during your review. If changes are made to the PR during the review process then your final comment will be the most important for gatekeepers. In this comment you should make it clear why any earlier review is still valid, or confirm that all requested changes have been addressed.

Code Review

Is the code of an acceptable quality?
Does the code conform to the coding standards?
Are the unit tests small and test the class in isolation?
If there is GUI work does it follow the GUI standards?
If there are changes in the release notes then do they describe the changes appropriately?
Do the release notes conform to the release notes guide?

Functional Tests

Do changes function as described? Add comments below that describe the tests performed?
Do the changes handle unexpected situations, e.g. bad input?
Has the relevant (user and developer) documentation been added/updated?

Does everything look good? Mark the review as Approve. A member of @mantidproject/gatekeepers will take care of it.

Gatekeeper

If you need to request changes to a PR then please add a comment and set the review status to "Request changes". This will stop the PR from showing up in the list for other gatekeepers.

peterfpeterson · 2025-02-10T14:20:02Z

This is no longer needed

github-actions · 2025-02-10T22:38:21Z

👋 Hi, @peterfpeterson,

Conflicts have been detected against the base branch. Please rebase your branch against the base branch.

Framework/DataHandling/inc/MantidDataHandling/DataBlockComposite.h

Framework/DataHandling/src/DataBlock.cpp

Framework/DataHandling/src/LoadILLSANS.cpp

Framework/DataHandling/src/LoadNexusProcessed.cpp

rboston628 · 2025-02-11T18:05:16Z

Framework/DataHandling/src/LoadNexusProcessed.cpp

@@ -295,7 +295,7 @@ Workspace_sptr LoadNexusProcessed::doAccelleratedMultiPeriodLoading(NXRoot &root
  NXDataSetTyped<double> errors(wsEntry, "errors");
  errors.openLocal();

-  const int nChannels = data.dim1();
+  const int nChannels = static_cast<int>(data.dim1());


Can this be declared int64_t?

There is too much pointer math in this section to edit as part of this PR. It deserves a harder look

rboston628 · 2025-02-11T18:29:51Z

Framework/Muon/src/LoadMuonNexus1.cpp

@@ -489,7 +489,7 @@ Workspace_sptr LoadMuonNexus1::loadDetectorGrouping(NXRoot &root, const Geometry
    NXInt groupingData = dataEntry.openNXInt("grouping");
    groupingData.load();

-    int numGroupingEntries = groupingData.dim0();
+    int numGroupingEntries = static_cast<int>(groupingData.dim0());


Could this not be int64_t or size_t?

I started down this path and it got ugly really fast

Framework/Muon/src/LoadMuonNexus2.cpp

rboston628 · 2025-02-11T18:33:31Z

Framework/DataHandling/src/LoadNexusProcessed.cpp

@@ -931,14 +931,14 @@ void LoadNexusProcessed::loadV3DColumn(Mantid::NeXus::NXDouble &data, const API:
  if (!columnTitle.empty()) {
    ColumnVector<V3D> col = tableWs->addColumn("V3D", columnTitle);

-    const int rowCount = data.dim0();
+    const int64_t rowCount = data.dim0();


Not related to your PR, but it annoys me that L848 and L851 above use dims[] and L934 uses dim0().

This might also need a cast operation, as above

I'm not messing with these

Framework/Nexus/src/NexusClasses.cpp

github-actions · 2025-02-13T14:32:26Z

👋 Hi, @peterfpeterson,

Conflicts have been detected against the base branch. Please rebase your branch against the base branch.

github-actions · 2025-02-18T22:00:14Z

👋 Hi, @peterfpeterson,

Conflicts have been detected against the base branch. Please rebase your branch against the base branch.

github-actions · 2025-02-20T13:09:25Z

👋 Hi, @peterfpeterson,

Conflicts have been detected against the base branch. Please rebase your branch against the base branch.

github-actions · 2025-02-20T18:58:51Z

👋 Hi, @peterfpeterson,

Conflicts have been detected against the base branch. Please rebase your branch against the base branch.

Since nexus actually works with int64_t, change the integer type in nxinfo to match

peterfpeterson changed the title ~~Change integer type~~ Change integer type of Mantid:Nexus::NXInfo Feb 7, 2025

peterfpeterson force-pushed the nxclass_int64 branch from 790de34 to bb5510e Compare February 7, 2025 21:52

peterfpeterson closed this Feb 10, 2025

peterfpeterson deleted the nxclass_int64 branch February 10, 2025 14:20

peterfpeterson restored the nxclass_int64 branch February 10, 2025 18:19

peterfpeterson reopened this Feb 10, 2025

peterfpeterson force-pushed the nxclass_int64 branch 3 times, most recently from fbca2b5 to 5c550b0 Compare February 10, 2025 20:15

github-actions bot added the Has Conflicts Used by the bot to label pull requests that have conflicts label Feb 10, 2025

peterfpeterson force-pushed the nxclass_int64 branch 2 times, most recently from e08c226 to 259a147 Compare February 11, 2025 14:49

peterfpeterson removed the Has Conflicts Used by the bot to label pull requests that have conflicts label Feb 11, 2025

peterfpeterson force-pushed the nxclass_int64 branch from 259a147 to cdb0bca Compare February 11, 2025 17:47

peterfpeterson marked this pull request as ready for review February 11, 2025 17:48

peterfpeterson added this to the Release 6.13 milestone Feb 11, 2025

peterfpeterson added the Maintenance Unassigned issues to be addressed in the next maintenance period. label Feb 11, 2025

rboston628 reviewed Feb 11, 2025

View reviewed changes

github-actions bot added the Has Conflicts Used by the bot to label pull requests that have conflicts label Feb 13, 2025

peterfpeterson added the Technical Debt Marks a piece of work to address technical debt introduced to solve a problem quickly label Feb 17, 2025

rboston628 mentioned this pull request Feb 17, 2025

Change NexusClasses integer types #38874

Merged

peterfpeterson force-pushed the nxclass_int64 branch from ec5a899 to 5576a64 Compare February 18, 2025 19:58

peterfpeterson removed the Has Conflicts Used by the bot to label pull requests that have conflicts label Feb 18, 2025

github-actions bot added the Has Conflicts Used by the bot to label pull requests that have conflicts label Feb 18, 2025

peterfpeterson force-pushed the nxclass_int64 branch from 5576a64 to 31efdde Compare February 19, 2025 13:16

peterfpeterson removed the Has Conflicts Used by the bot to label pull requests that have conflicts label Feb 19, 2025

peterfpeterson force-pushed the nxclass_int64 branch from 31efdde to 2124ac1 Compare February 19, 2025 20:52

github-actions bot added the Has Conflicts Used by the bot to label pull requests that have conflicts label Feb 20, 2025

peterfpeterson force-pushed the nxclass_int64 branch from 2124ac1 to 3f95cc7 Compare February 20, 2025 16:43

github-actions bot removed the Has Conflicts Used by the bot to label pull requests that have conflicts label Feb 20, 2025

rboston628 mentioned this pull request Feb 20, 2025

More changes to proper integer type #38939

Open

github-actions bot added the Has Conflicts Used by the bot to label pull requests that have conflicts label Feb 20, 2025

peterfpeterson force-pushed the nxclass_int64 branch 2 times, most recently from 9734f34 to c01c3bf Compare February 20, 2025 20:46

peterfpeterson added 3 commits February 20, 2025 16:16

Change integer type

16d8c56

Since nexus actually works with int64_t, change the integer type in nxinfo to match

Rename typedef to be clearer that it is a scalar

e4fec36

Change integer types in more places

9a19ed1

peterfpeterson force-pushed the nxclass_int64 branch from c01c3bf to 9a19ed1 Compare February 20, 2025 21:16

peterfpeterson removed the Has Conflicts Used by the bot to label pull requests that have conflicts label Feb 20, 2025

rboston628 approved these changes Feb 20, 2025

View reviewed changes

rosswhitfield merged commit d17c62a into mantidproject:main Feb 20, 2025
10 checks passed

rosswhitfield mentioned this pull request Feb 20, 2025

Change integer type of Mantid:Nexus::NXInfo - ornl-next #38944

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change integer type of Mantid:Nexus::NXInfo #38805

Change integer type of Mantid:Nexus::NXInfo #38805

peterfpeterson commented Feb 7, 2025 •

edited

Loading

peterfpeterson commented Feb 10, 2025

github-actions bot commented Feb 10, 2025

rboston628 Feb 11, 2025

peterfpeterson Feb 11, 2025

rboston628 Feb 11, 2025

peterfpeterson Feb 11, 2025

rboston628 Feb 11, 2025

rboston628 Feb 11, 2025

peterfpeterson Feb 11, 2025

github-actions bot commented Feb 13, 2025

github-actions bot commented Feb 18, 2025

github-actions bot commented Feb 20, 2025

github-actions bot commented Feb 20, 2025

Change integer type of Mantid:Nexus::NXInfo #38805

Change integer type of Mantid:Nexus::NXInfo #38805

Conversation

peterfpeterson commented Feb 7, 2025 • edited Loading

To test:

Reviewer

Code Review

Functional Tests

Gatekeeper

peterfpeterson commented Feb 10, 2025

github-actions bot commented Feb 10, 2025

rboston628 Feb 11, 2025

Choose a reason for hiding this comment

peterfpeterson Feb 11, 2025

Choose a reason for hiding this comment

rboston628 Feb 11, 2025

Choose a reason for hiding this comment

peterfpeterson Feb 11, 2025

Choose a reason for hiding this comment

rboston628 Feb 11, 2025

Choose a reason for hiding this comment

rboston628 Feb 11, 2025

Choose a reason for hiding this comment

peterfpeterson Feb 11, 2025

Choose a reason for hiding this comment

github-actions bot commented Feb 13, 2025

github-actions bot commented Feb 18, 2025

github-actions bot commented Feb 20, 2025

github-actions bot commented Feb 20, 2025

peterfpeterson commented Feb 7, 2025 •

edited

Loading