Ticket #9376 (closed: fixed)

Opened 6 years ago

Last modified 5 years ago

Create mantid dataset for published data

Reported by: Jay Rainey Owned by: Jay Rainey
Priority: major Milestone: Release 3.2
Component: Framework Keywords: ICAT
Cc: Blocked By:
Blocking: Tester: Peter Parker

Description (last modified by Jay Rainey) (diff)

Currently, when publishing a datafile to the archives it is stored in the Default (first) dataset alongside the raw experiment data. Instead, analyzed data published from Mantid should be stored in a separate dataset (named mantid).

To achieve this, I need to:

  1. Loop over the datasets for the specific investigation.
  2. Set the datasetID to the dataset ID of the mantid dataset if it exists.
  3. When the loop is complete, check if datasetID is still it's default value. If it is, then no mantid dataset was found. As no dataset was found, we need to create one via createMantidDataset*.

* This method will create a dataset named mantid for a given investigationID. I will need to call isAccessAllowed prior to create to verify if I can create the dataset. If I cannot then I should return -1 and output a notice().

Change History

comment:1 Changed 6 years ago by Jay Rainey

  • Description modified (diff)

comment:2 Changed 6 years ago by Jay Rainey

  • Description modified (diff)

comment:3 Changed 6 years ago by Jay Rainey

  • Blocking 8538 removed

comment:4 Changed 6 years ago by Owen Arnold

  • Status changed from new to assigned

comment:5 Changed 6 years ago by Jay Rainey

  • Status changed from assigned to inprogress

Add generic isAccessAllowed method. Refs #9376.

Changeset: 459c6b01b5ca1950a2932e17ade79f4886281727

comment:6 Changed 6 years ago by Jay Rainey

Use isAccessAllowed for publish GUI. Refs #9376.

Changeset: e753685a3932579bd3a1c223d561b0efa5779ae1

comment:7 Changed 6 years ago by Jay Rainey

Add a method to search ICAT. Refs #9376.

Changeset: 6848ee831f915cb238f620360acc36166032c589

comment:8 Changed 6 years ago by Jay Rainey

Use new search method. Refs #9376.

Changeset: 8cc230e68bff91581e405b38ee780854c0dc43cc

comment:9 Changed 6 years ago by Jay Rainey

Forgot to add search to getMantidDatasetId. Refs #9376.

Changeset: 1aa471c75442a95e4786f9db4487d073dd193e55

comment:10 Changed 6 years ago by Jay Rainey

Tidy return logic. Refs #9376.

Changeset: c2e738a2345dc9965b25b11c8318e4411c730ae1

comment:11 Changed 6 years ago by Jay Rainey

Removed unnecessary icat4 namespace prefixes. Refs #9376.

Changeset: 21e18466616766eb95a9016b0a413905031bead1

comment:12 Changed 6 years ago by Jay Rainey

Remove debug statement. Refs #9376.

Changeset: 61a88e9cc900ac82c5d9dd02690fcf311a1f7295

comment:13 Changed 6 years ago by Jay Rainey

Move method to align with header order. Refs #9376.

Changeset: eb99ecc435d563d5c6921ca350c1942f410cfbee

comment:14 Changed 6 years ago by Jay Rainey

Added createMantidDataset method. Refs #9376.

Changeset: 1bc871a666e800197da79c619d0af94edf544c8a

comment:15 Changed 6 years ago by Jay Rainey

Create a mantid dataset if none exist. Refs #9376.

Changeset: 2a4fe517a558b7ec3f0a7c93b1d0801f58b2d352

comment:16 Changed 6 years ago by Jay Rainey

  • Description modified (diff)

comment:17 Changed 6 years ago by Jay Rainey

  • Status changed from inprogress to verify
  • Resolution set to fixed

Added debugging statements for publishing. Refs #9376.

Changeset: dafe1289b589b0503798adc45f2b351b78a6d63a

Last edited 6 years ago by Jay Rainey (previous) (diff)

comment:18 Changed 6 years ago by Jay Rainey

Overview

This ticket deals with separating analyzed data and experimental data in ICAT. I have opted to create a new dataset (named mantid) for the investigation the user wants to relate files to when they try to publish data. If the mantid dataset already exists, then we PUT the files there.

Testing publishing functionality

Note: You must use the mantid ICAT test account to verify this ticket as it has an investigation in my data.

  1. Open the Search GUI and check My Data only. Ensure that Instrument name is empty. (This way it will return all investigations).
  2. Copy the Investigation ID of Mantid Test Investigation. (It's 1193002).
  3. From the Algorithms menu enter CatalogGetDataSets.
  4. Enter the copied investigation ID (1193002) into the InvestigationID field, and input testingTicket into the OutputWorkspace field and click Run.
  5. Right click the workspace and click Show data. Observe that there are two datasets for this specific investigation, and take note of the Number of datafiles in the mantid dataset.
  6. Open the publish dialog and publish a datafile.
  7. Execute CatalogGetDataSets again as above, and observe that the Number of datafiles in mantid has increased. This proves that datafiles are not published to the mantid dataset.
  8. Use the Search GUI to find the datafile you just published, and load it into Mantid.

Testing searching functionality

As the createMantidDataset performs several actions (to ensure create is successful and valid) I re-factored the searching of ICAT into a separate method named performSearch. As such, all ICAT functionality that performs searches will also have to be tested, which includes:

  1. Instrument name is populated correctly via CatalogListInstruments on the ICAT GUI.
  2. Investigation type is populated correctly via CatalogListInvestigationTypes on the ICAT GUI.
  3. Perform searches on the ICAT GUI works correctly.
  4. Publishing datafiles works correctly (getMantidDatasetId is used to search for the dataset ID of the investigation to publish to).
  5. CatalogMyData needs to be run via the Algorithms menu.

Perform a code review to ensure the changes made are sensible.

Last edited 6 years ago by Jay Rainey (previous) (diff)

comment:19 follow-up: ↓ 26 Changed 6 years ago by Russell Taylor

There's an Intel compiler warning here that shows why it's better to always put brackets around if-else clauses even if they're only one line:

http://builds.mantidproject.org/job/develop_incremental/label=osx-10.8-build/110/warnings24Result/?

comment:20 Changed 6 years ago by Russell Taylor

  • Status changed from verify to reopened
  • Resolution fixed deleted

comment:21 Changed 6 years ago by Jay Rainey

  • Status changed from reopened to inprogress

Add braces to address Intel compiler warning. Refs #9376.

Changeset: 770ab5bda8cdecbb2ae9bed32bd86a0d6b3f374c

comment:22 Changed 6 years ago by Jay Rainey

  • Status changed from inprogress to verify
  • Resolution set to fixed

comment:23 Changed 6 years ago by Peter Parker

  • Status changed from verify to verifying
  • Tester set to Peter Parker

comment:24 Changed 6 years ago by Peter Parker

Changes are sensible. New functionality works as expected and the existing search functionality seems to be unaffected.

Closing.

comment:25 Changed 6 years ago by Peter Parker

  • Status changed from verifying to closed

Merge remote-tracking branch 'origin/feature/9376_icat_create_mantid_dataset'

Full changeset: 0455fbf70e139c22cde2c2b368c1de8fcc280d98

comment:26 in reply to: ↑ 19 ; follow-up: ↓ 27 Changed 6 years ago by Peter Parker

Replying to Russell Taylor:

There's an Intel compiler warning here that shows why it's better to always put brackets around if-else clauses even if they're only one line:

http://builds.mantidproject.org/job/develop_incremental/label=osx-10.8-build/110/warnings24Result/?

I'm not wholly convinced this is an argument against bracket-less condition statements (which, I'll admit, I like and use a lot). Surely the code before and after Jay's fix is logically equivalent as per the standard and so it is the compiler at fault here?

Are there other reasons why we should be avoiding them? It seems to me they're as easy / hard to read as condition statements in Python, and that always adding brackets as a rule takes more effort than only adding brackets when they're necessary. I am open to other arguments, though.

comment:27 in reply to: ↑ 26 Changed 6 years ago by Jay Rainey

Replying to Peter Parker:

The Intel error above was not related to braces. I wrote a short-hand if/else and forgot to put a return statement in the else after I commented out the return method (as it did not exist on develop, e.g. something like this).

comment:28 Changed 6 years ago by Russell Taylor

The code that triggered the warning looked like this:

905       if (icat.search(&request,&response) == SOAP_OK)
906         searchResults = response.return_;
907       else
908         // throwErrorMessage(icat);
909 
910       return searchResults;

Because whitespace doesn't matter, the return statement 'belonged' to the else clause, and the if path was then missing any return statement, triggering the warning.

If there had been braces then the commenting out would have left an empty else clause, so perhaps my comment should have also been that tickets shouldn't be closed with commented out code (though in this case I think develop had somehow diverged anyway so this code wasn't in the branch).

I tend not to like brace-less single line if-elses because of the risk that someone will add a line and it will not do what they expect. What I often do for these is put them on the same line, i.e.:

  if (test) doSomething;
  else doSomethingElse;

comment:29 Changed 6 years ago by Peter Parker

Okay, thanks for the clarification guys.

comment:30 Changed 5 years ago by Stuart Campbell

This ticket has been transferred to github issue 10219

Note: See TracTickets for help on using tickets.