Ticket #8560 (closed: fixed)
Refl_gui Improvements - RB functionality
Reported by: | Keith Brown | Owned by: | Keith Brown |
---|---|---|---|
Priority: | blocker | Milestone: | Release 3.1 |
Component: | Reflectometry | Keywords: | |
Cc: | Blocked By: | #8531, #8532, #8549 | |
Blocking: | #7377, #8747, #8750 | Tester: | Samuel Jackson |
Description
There is a QLineEdit widget on the gui marked "RB" the functionality of this was broken between 2.6 and 3.0, make it produce the same behaviour as 2.6
Change History
comment:2 Changed 7 years ago by Keith Brown
- Blocked By 8531, 8532, 8549 added
Added blocking tickets as they need to be pushed to master before I can finish this ticket as I'll need to merge them in.
comment:3 Changed 7 years ago by Keith Brown
Refs #8560. Changed script to hold a single tuple instead of map
Latest_isis_runs.py no longer cares about runs past the most recent.
It used to hold a dictionary of <cycle,[journal_path,cycle_path]> where it not holds only the [journal_path,cycle_path] of the most recent cycle
It will also return a list of strings in the form of [run_no]: [title]
Changeset: 9677b0eafb819f1a98dbc3547655bf23956accd2
comment:4 Changed 7 years ago by Keith Brown
Refs #8560. Functionality fixed, waiting for blocking tickets
The script now returns the data in the format we want, and does a check to make sure the user has entered only an integer to save checking for an eID that won't exist
The ui has had a minor tewk in that the central widget has had it's name changed and a sensible name given to the layout it hosts. The ListWidget has also been expanded slightly. Compared to the old functionality,(as of 2.6 when this wokred last) the list box will no logner resize when given data, scrollbars will be used instead.
This ticket is now wating for #8531, #8532 and #8549 to be pushed to master so i can merge them in and make sure that the functionality doesn't break.
Changeset: 28d34917bf08ba5ff61e33afa06eba1a3ffdceae
comment:5 Changed 7 years ago by Keith Brown
Refs #8549. Working on Latest_isis_runs
As the scientists don't seem interested in other cycles, the latest_isis_runs script is bieng altered to return a list rather than a map
Changeset: c583d2a12fc108882bf7e95580452017cce75eaf
comment:6 Changed 7 years ago by Keith Brown
above commit referenced the wrong ticket and has been copied over here, this should have been the first commit on the ticket
comment:7 Changed 7 years ago by Keith Brown
Refs #8560. Removed comboCycle and added blank RB case
The comboWidget comboCycle hasbeen removed from the ui, and it's related code deleted or disabled.
If the RB box is empty, the listWidget will be populated with all runs form that cycle.
Changeset: 2533319cf9822aa5842ebb4b32ea1a603a4a3d23
comment:8 Changed 7 years ago by Keith Brown
Refs #8560. Merge remote-tracking branch 'origin' into feature/8560_Refl_gui_RB_functionality
Changeset: 05415dc0e1a23865d8d940182811844d8a49aa65
comment:9 Changed 7 years ago by Keith Brown
Refs #8560. Merge origin/master into feature/8560
Previous ticket has now been merged into this ticket to reduce the chance of further merge conflicts
Changeset: 491c8472c12b6bbec76453952b2308d368a52ff3
comment:10 Changed 7 years ago by Keith Brown
Refs #8560. Added a minimum size to the QLineEdits
Not related to the core issue, but samll enough to include.
It was noticed on Mac that the RB lineEdit was rather small on the defualt window size, so I've added a minimum width to both LineEdits.
Changeset: c232f67047f229e270ef87c9470a91378f2137f6
comment:11 Changed 7 years ago by Keith Brown
Refs #8560. Fixed intrument selection after breaking in another ticket
When I changed the signal connections from the old C++ style to the new pyqt style notation i inadvertently broke the instrument selection combobox.
This was because the default "Activated" signal carried the index of the new item, rather than the new contents like it used to. The same method couldn't be kept as combobox would only return unicode strings, which the config manager didn't like as they weren't compatible with C++ std::string.
The solution was to a new class-member level list of instruments in all caps and use it to populate the listbox in the subclass (removing the hard coded values from the UI file) and set the config manager meaning that the values used would be python strings (compatible with C++ std::string)
Changeset: f927e5a226f04308f02e966bef5ab4d9c80d7ef7
comment:12 Changed 7 years ago by Keith Brown
- Status changed from inprogress to verify
- Resolution set to fixed
I believe I've accomplished what was asked by this ticket with a few other related fixes.
Points to test:
- You can enter an RB and get runs with a corresponding Experiment ID back populating the listbox.
- The instrument selection works as expected, populating the listbox with all the recent cycle runs for that instrument when the instrument is switched (this will also clear the RB box)
- The Cycle selection box is gone.
- On a Mac (and perhaps linux) the line edit widgets are now a sensible size.
comment:13 Changed 7 years ago by Keith Brown
Refs #8560. Merge remote-tracking branch 'origin' into feature/8560_Refl_gui_RB_functionality
Changeset: da96ae81a99949365117fc48cbc48d398b5da005
comment:14 Changed 7 years ago by Keith Brown
Refs #8560 develop. Merge branch 'feature/8560' into develop
Conflicts:
Code/Mantid/scripts/Interface/ui/reflectometer/refl_gui.py Code/Mantid/scripts/Interface/ui/reflectometer/refl_window.py
Changeset: 5cda62d38f8829c38c3d54611c8e80e134aeea37
comment:15 Changed 7 years ago by Keith Brown
- Status changed from verify to reopened
- Resolution fixed deleted
Met with reflectometry guys, been told two things:
- They are interested in all runs, all the time. So RB functionality should search them all, rather than a specific cycle
- They want it fast, so the old index-based (rather than purely DOM based) functionality is there to stay as the python module combined with the way I had to search was VERY slow
comment:16 Changed 7 years ago by Keith Brown
- Status changed from reopened to inprogress
Refs #8560. Corrected RB functionality
Lots of changes:
- The RB search is no longer triggered by textRB's editingFinished signal. It's now linked to textRB's returnPressed and the new buttonSearch's clicked. So this will now only search when enter is pressed in the text box or the search button is clicked.
- A search won't be attempted if the RB box is empty
- transfer() and the RB search will display an error message in a messagebox if something goes wrong.
- The main window now has a status bar, used for showing search status.
- The RB search no longer uses cycles, and will search every journal from the beggining of time to the present day
- The RB search is much faster (it's practically instant) due to using summary.txt (located alongside the xml journals) instead of the xml journals. It's guessed that the speed gain has come from only opening one file over a network rather than many, the xml module might have also been slightly to blame.
- The RB search no longer adds cycle directories to the managed user directories - this is a side effect of using the summary.txt as that doesn't include the cycle so the fucntion can't add the cycle directory.
A decision is being made on which direction to take in terms of seraching, as both xml and txt methods are in the code and it's a case of changing a single function call to switch them. txt is instant, but doesn't add cycles to the managed directories, while xml takes about 40 seconds but adds cycle directories to the managed directories
Changeset: 648a6adcb1adca98e71c794168f2140da8f85700
comment:17 Changed 7 years ago by Keith Brown
- Status changed from inprogress to verify
- Resolution set to fixed
For now, search is remaining using the txt file
Opening for testing.
comment:18 Changed 7 years ago by Owen Arnold
- Status changed from verify to reopened
- Resolution fixed deleted
Keith. I've been speaking to the reflectometry scientists and we think that the best thing to do here would be to use the xml version of the lookup, but limit the number of cycles that it goes backwards when it does the search to 4 by default. We could then provide a spin box next to the RB search to increase/decrease the number of cycles to look back in. That way we would get fast searching, but would also be able to also automatically have them added to the search directories.
Given you're previous comment (comment 17) this doesn't seem like it would be particularly hard to do, or take very long to do.
comment:19 Changed 7 years ago by Keith Brown
Referenced wrong ticket in commit. This is the ticket it actually corresponds to.
Refs #8633. Changed search to use xml and a fixed cycle depth
The search now uses xml again and is fixed to 1 cycle into the past (although this will be fixed to a spinbox in another ticket to allow the user to set how many cycles into the past they want to search)
Changeset: 8328f140d06a78b7b24d3a1c48e168881b7e2fdb
comment:20 Changed 7 years ago by Keith Brown
- Status changed from reopened to verify
- Resolution set to fixed
comment:21 Changed 7 years ago by Keith Brown
Refs #8560. Updated for the new API
The provided file used the old API. The code has been updated to use the new API.
The file also needed some global functions defined in it too
Changeset: 90aae6526aa6caa6349a993cc04ccb75a0d479d8
comment:22 Changed 7 years ago by Keith Brown
Above commit referenced wrong ticket. Should have referenced #8633 instead.
comment:23 Changed 7 years ago by Owen Arnold
- Status changed from verify to verifying
- Tester set to Owen Arnold
comment:24 Changed 7 years ago by Owen Arnold
- Status changed from verifying to reopened
- Resolution fixed deleted
comment:25 Changed 7 years ago by Keith Brown
- Status changed from reopened to verify
- Resolution set to fixed
This is fixed, the search now uses xml. was reopened due to a couple of commits with wrong references
comment:27 Changed 7 years ago by Owen Arnold
- Status changed from verifying to reopened
- Resolution fixed deleted
Need the spin box implemented here otherwise the changes will not be useful and scientist/users are looking at the nightly builds.
comment:28 Changed 7 years ago by Keith Brown
- Status changed from reopened to inprogress
Refs #8560. Added spinbox for RB search depth
I've added a spinbox for setting the depth of the search and made a few general improvments to the UI at the same time.
The widgets are now spread across two rows as they would have been cramped with the new widgets.
The Instrument combobox now has a label.
Buddies have been set, each label is now paired with a widget.
Tab order is now set properly.
Changeset: 4791612e7cd1b23f27250ef848357c391fd0ab55
comment:29 Changed 7 years ago by Keith Brown
Previous commit also removed option 3 from the polarisation correction combobox in preperation for another ticket
comment:30 Changed 7 years ago by Keith Brown
Refs #8560. Spin box now sets search depth.
The spin box value is now attached to the call to getJournalRuns, so the search depth can now be adjusted.
Changeset: 18373c8c7a13ea3fa48dae8f6ecce0b65f138338
comment:31 Changed 7 years ago by Keith Brown
- Status changed from inprogress to verify
- Resolution set to fixed
comment:32 Changed 7 years ago by Keith Brown
- Status changed from verify to reopened
- Resolution fixed deleted
Reopening as i forgot to attach a signal
comment:33 Changed 7 years ago by Keith Brown
I thought a qspinbox also had the returnpressed signal, but it doesn't. So I'm not attaching anything else
comment:34 Changed 7 years ago by Keith Brown
- Status changed from reopened to verify
- Resolution set to fixed
comment:36 follow-up: ↓ 43 Changed 7 years ago by Owen Arnold
- Status changed from verifying to reopened
- Resolution fixed deleted
Unfortunately I have to reopen this. Here are some specific points that need to be addressed.
- No indication that the search is happening. Not even a log message to indicate success/failure.
- The expriement id 24088082 is not found even though it does exist and should contain INTER00013470.nxs INTER00013460.nxs etc. I set the number of cycles to to search to 10. How did you test this?
You should be able to verify that 24088082 is the RB number that corresponds to the runs from this cycle by looking at the journal.xml files.
comment:37 Changed 7 years ago by Keith Brown
Looking at the journals, INTER00013470.nxs and INTER00013460.nxs come under RB/Experiment ID 1120015 and I've checked and made sure that that number brings up those runs and more besides. And assuming that the RB/Experiment IDs are sequential, the most recent run has only 1320341 so I doubt 24088082 will bring up anything
I will however print out more notifications for search start and completion.
comment:38 Changed 7 years ago by Keith Brown
- Status changed from reopened to inprogress
Refs #8560. Added more prints to the log
Added more print notices for search start and end.
Changeset: fb0da72f1840e56a4778473130d606aa67f80571
comment:39 Changed 7 years ago by Keith Brown
- Status changed from inprogress to verify
- Resolution set to fixed
comment:41 Changed 7 years ago by Keith Brown
- Priority changed from critical to blocker
- Blocked By 8721 removed
This ticket actually fixes the problem from 8721 as well, and it's rather urgent as the bug limits the use of the gui rather heavily
comment:43 in reply to: ↑ 36 Changed 7 years ago by Owen Arnold
Replying to Owen Arnold:
Unfortunately I have to reopen this. Here are some specific points that need to be addressed.
- No indication that the search is happening. Not even a log message to indicate success/failure.
- The expriement id 24088082 is not found even though it does exist and should contain INTER00013470.nxs INTER00013460.nxs etc. I set the number of cycles to to search to 10. How did you test this?
You should be able to verify that 24088082 is the RB number that corresponds to the runs from this cycle by looking at the journal.xml files.
My mistake: 1320341 is the actual RB number. 24088082 is the internal ICAT primary key for this record.
comment:44 Changed 7 years ago by Owen Arnold
Tester:
Also -
- Check that the Instrument selection now works (ask Keith)
- Check that 'Process' still works. Keith should be able to demonstrate this working and producing plots.
comment:45 Changed 7 years ago by Owen Arnold
- Status changed from verifying to verify
- Tester Owen Arnold deleted
Samuel will test this.
comment:46 Changed 7 years ago by Samuel Jackson
- Status changed from verify to reopened
- Resolution fixed deleted
Couple of small issues before passing this ticket:
- There's still no output to tell me when the search is working. This could just be something as simple as telling me what cycle we're currently looking at so I can see progress is being made.
- Perhaps a log message actually confirming a success of failure, rather than just saying "complete".
- Placing an upper bound on the number of cycles searched. While this doesn't break functionality, it does seem odd that I can search an infinite number of cycles.
- A few minor bits of python re-factoring as discussed.
But otherwise this seems to work as intended. Complete these minor points and I will be happy to pass this ticket.
comment:47 Changed 7 years ago by Owen Arnold
Comments above seem sensible, particularly around progress reporting and success/failure in searching.
comment:48 Changed 7 years ago by Keith Brown
- Status changed from reopened to inprogress
Refs #8560. Added maximum to spinbox and improved complete message.
The cycle depth spinbox is now more inteligent, its max value is now set from the number of cycle journals logged in the main jounral file
The search complete message now contains a value for the ammount of results found. Searches that yeild nothing are still deemed complete as it searched the amount it was told to but nothing existed.
Refactored a few lines in the txt search method to explain cnstant values, and added a comment about a concatentaion line.
Changeset: 772b33ae5efef33f02647d1d6de1d731a4301329
comment:49 Changed 7 years ago by Keith Brown
I attempted to add more output logs when searching, but hit a problem where the messages weren't appearing until the search completed, thus voiding the effort.
This is the the same snag I encountered when I tried setting messages on the statusbar the recommended way (using addWidget()/removeWidget() rather than showMessage()/clearMessage()) and the reason why the listbox doesn't appear to clear before the search even though the call is made before the search begins.
comment:50 Changed 7 years ago by Keith Brown
- Status changed from inprogress to verify
- Resolution set to fixed
I've fixed all I can of the points that the tester mentioned, and those I haven't I've explained.
comment:51 Changed 7 years ago by Samuel Jackson
- Status changed from verify to verifying
- Tester set to Samuel Jackson
comment:52 Changed 7 years ago by Samuel Jackson
- Status changed from verifying to reopened
- Resolution fixed deleted
I'm afraid I now get the following issue when running git test start:
ERROR: Branch 'feature/8560_Refl_gui_RB_functionality' has not been fully merged to develop, meaning it is not possible to know whether the code is valid on all environments. Contact Keith Brown on keith.brown@stfc.ac.uk (or via skype) and ask them to merge the code to develop using 'git checkbuild' from their branch feature/8560_Refl_gui_RB_functionality and let you know when it passes testing on the buildservers so you can start testing.
Fix and I'll test.
comment:53 Changed 7 years ago by Keith Brown
Basically I forgot to checkbuild. Just as well as I found a point where an error could throw and fixed it.
comment:54 Changed 7 years ago by Keith Brown
- Status changed from reopened to inprogress
Refs #8560. Fixed a point where an exception could throw.
If a journal had no 'isis_cycle' tag then the search would crash. The serach now skips that journal if that is the case.
Changeset: 40adf777b1262f4fb40133716a4e9cefd176b390
comment:55 Changed 7 years ago by Keith Brown
- Status changed from inprogress to verify
- Resolution set to fixed
comment:56 Changed 7 years ago by Samuel Jackson
- Status changed from verify to reopened
- Resolution fixed deleted
Unfortunately, I've still having problems with this ticket:
- Attempting to open a table file throws an error.
- Attempting to press process with manually entered values causes and error on line 496 of refl_gui because it's checking for a float type, but finding a nump.float64 type.
comment:57 Changed 7 years ago by Keith Brown
Fixing former error now
Latter error was already present in script as i've not touched that method in all the time I've been working on this gui. I'll open a new ticket for it.
comment:58 Changed 7 years ago by Keith Brown
- Status changed from reopened to inprogress
Refs #8560. Fixed a widget naming issue
The error was because the open dialog was parented to a control that had been renamed. Fixed naming issues
Changeset: 82a0ffff7d9c803da44baddc4ad54229c52112d9
comment:59 Changed 7 years ago by Keith Brown
Refs #8560. fixed saving issue
The save action haddn't been defined right. I'd forgotten to add self.
Changeset: 52dbad48f757f83ef3b3eef489b462064ceee444
comment:60 Changed 7 years ago by Keith Brown
Refs #8560. Merge remote-tracking branch 'origin/master' into feature/8560_Refl_gui_RB_functionality
Changeset: 3de264da3bdfd73b6441fb94d740a9e981e9f50c
comment:61 Changed 7 years ago by Keith Brown
Merged master in to make sure nothing else is gonna break as well as make sure there were no conflicts as this ticket has been open a while.
comment:62 Changed 7 years ago by Keith Brown
Note that this ticket contains the following fixes in addition to the RB behaviour:
- The Open dialog is now parented properly
- The search no longer crashes on a search which would look at a journal with no cycle identification entries - this issue was observed with CRISP on a depth of 22
- the save action now works after it had been broken by the lack of 'self'
- The Instrument selection will change the instrument properly
comment:63 Changed 7 years ago by Keith Brown
Refs #8560. Fixed statusbar messages
Discovered that the messages in the statusbar would dissapear when opening a menu. This was because of the temporary messages is was using.
The statusbar now has a standard message "Ready" and only uses temporary messages for the search notification.
Changeset: bebfe5093a9991e9cf891b1e55f3ca5c16c387fb
comment:64 Changed 7 years ago by Keith Brown
- Status changed from inprogress to verify
- Resolution set to fixed
comment:66 Changed 7 years ago by Keith Brown
Refs #8560 Fixed another Parenting issue
There was a spelling error when trying to parent a fileDialog.
Changeset: 87f24678f0cc662536725897f5bc2e19343dba53
comment:67 Changed 7 years ago by Keith Brown
- Status changed from verifying to reopened
- Resolution fixed deleted
comment:68 Changed 7 years ago by Keith Brown
- Status changed from reopened to verify
- Resolution set to fixed
comment:72 Changed 7 years ago by Samuel Jackson
- Status changed from verifying to closed
Merge remote-tracking branch 'origin/feature/8560_Refl_gui_RB_functionality'
Full changeset: 3a4c69b5876d8451b6ed109b7b231f9eea236776
comment:73 Changed 7 years ago by Samuel Jackson
I quickly ran though the previous testing requirements as well as the new changes. While these changes should still be tested on Linux/Mac, it appears to be working as intended on Windows. A minor issue identified (but that can be fixed in another ticket) was:
- trimming whitespace from investigation numbers. I accidentally copy/pasted an investigation number along with a space and was confused as to why it failed to find anything.
comment:74 Changed 7 years ago by Keith Brown
Refs #8560. Resolve Merge conflict between feature/8532 and develop
Changeset: d2d043081386226f4404c578e6e7bd164513d518
comment:75 Changed 7 years ago by Keith Brown
Refs #8560 develop. Merge branch 'feature/8560' into develop
Conflicts:
Code/Mantid/scripts/Interface/ui/reflectometer/refl_gui.py Code/Mantid/scripts/Interface/ui/reflectometer/refl_window.py
Changeset: 5cda62d38f8829c38c3d54611c8e80e134aeea37
comment:77 Changed 5 years ago by Stuart Campbell
This ticket has been transferred to github issue 9404
Refs #8560. Resolve Merge conflict between feature/8532 and develop
Changeset: d2d043081386226f4404c578e6e7bd164513d518