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

Tabs, Batch Processing and 2D data slicing and processing for pr #2181

Open
wants to merge 150 commits into
base: release_6.0.0
Choose a base branch
from

Conversation

ru4en
Copy link

@ru4en ru4en commented Sep 7, 2022

Tabs, Batch Processing, Plots, Tables and 2D data slicing & processing for Pr

This is the work I’ve done during my summer placement. Most of the changes I have made are in the Inversion Perspective directory. I’ve attached A short document describing the changes/feature I’ve implemented.

Features:

  • Tabs
  • 1D Batch Processing
  • 2D slicing and Batch Processing of slices
  • 2D plot and 1D slice Plots
  • Batch Table
  • CSV exporting and Plotting on the Batch Table window
  • Minor changes such as fixing the Dropdown menu and remembering Parameters

Find more Information in the PDF bellow.
Notes and Reference Document for Pr interface DRAFT.pdf

ru4en added 30 commits July 1, 2022 15:09
issue #1448 - Enable batch processing for P(r)

NOTE: This issue is not yet fixed.

Feature:

Selected data sent from the load data can now be sent to Pr window and a new tab will appear for each data set. Although the goal is batch calculate all data and neatly display in a table the current “non batch” tab method can be used for quick references of calculations for each file. In the nect commit I intend to implement a batch tab. Notes for those are in the bottom.

Dev note

-  1) Block automatic calculation after data is sent to Pr prospective whilst batch mode is deselected. (Allow users to hit calculate) Or block graphs from showing automatically as it clusters the interface.

-  2) Fix batch Mode
o  When batch mode is selected create only one tab (similar to fitting)
o  New interface for batch / same interface could be used with the addition of “Fit” / ” Graph” button conjunction with the 1st TODO.

3) Clean up Code. The current files ive modified/created are messy. Also comment code
…w and efficient. also a way to display/export all the data in a table is still required
Currently, Tabs works. Batch still has a little ironing out to do. mainly its interaction with setCurrentData(). Additionally, The data from Batch still needs to be sent to a Table and eventual reported/exported.
files calculated using Pr can now be batch Processed and saved to a table. However, a lot of work is still left to fix optimise and automate specific parts of this feature.

things still left to fix
- [ ] calculate all files before showing the table
- [ ] perform calculations without stop showing the plots as it slows down and clutters the interface
- [ ] optimise, clean and comment the code
Files imported into Pr interface can now automatically be processed and outputs displayed in a table. Code still requires to be optimised and cleaned. A better way to update the table is also required.

things still left to fix
- [x] calculate all files before showing the table
- [x] perform calculations without stop showing the plots as it slows down and clutters the interface
- [ ] optimise, clean and comment the code
However, im still getting errors with displayChange and setCurrentData even though the calculations and outputs are successful.
 Batch Processing still not working properly. Cleaned a bit of the code(That I was working on). 2D data shows a plot of the data however calculations still cannot be made. in the process of coding the bit gets a slice of the 2D data.
when 2D data is loaded into Pr interface a raw data plot should pop up. Using the Pr interface, parameters for the conversions can be inputted and when calculated should update the graphs and calculate the Pr values for those specific slices.
… be plotted on a single plot.

However, currently only one of the slices is calculated using Pr, this will be changed (Hopefully) in the next commit so that all sliced can be processed together similarly to 1D Batch and its outputs displayed in a table.
the table populates the slices created and the button when pressed need to show plots of the slice.

calculate all button need to also be fixed. the data cannot be read as its not in the right object.
…dating the gui and getting/setting parameters.
 Furthermore, the data output ont the batch table will be from self._calculator. this means It's flexible for further expansions and is just as reliable as the gui. Although a few Issues still remain during calculation, one being Q range being set to 0 or bellow which causes the data set to not be calculated and the calculations of the previous data to be set for the 'current' data set (Hence you might see duplicate rows with different file names). In my previous commit in the branch 1D data could not be uploaded for individual calculation that bug has now been fixed. Code still needs to be cleaned up and commented.
this is a bit of a hacky way of getting it to work and is just to get an idea of how it might work. things to improve upon are :-

- [ ] Getting the table in GridPanel.py to work flexibly for both 1D batch aswell as 2D slices.

- [ ] cleaning up slices()

- [ ] making showBatchOutput() more portable

- [ ] creating batch like datalist support for 2D slices. (so that it can be batch processed just like 1D batch files)

- [ ] SORTING OUT THE ERRORS

- [ ] (Might need help) Fix the Wierd Parameter issue. (Currently set/default parameters change to estimates which makes it hard to use reliabely. Background, Qmin and Qmax is also not working as expected, it sets itself to a value bellow 0 causing it to not calculate for that slice.)
need to test, clean up code, and make sure that this doesn't affect 1D batch processing
…ionally, Q min and Q Max can be seen in the Batch results panel.

However, further testing and Code cleaning is required. Newly created pop up windows such as the batch results, 1D slice plot, and 2D image plots need to be connected with the interface and made to fit in withing the UI.
…ionally, Q min and Q Max can be seen in the Batch results panel.

However, further testing and Code cleaning is required. Newly created pop up windows such as the batch results, 1D slice plot, and 2D image plots need to be connected with the interface and made to fit in withing the UI.
… Pr fit and I(q) plots.

This Version should somewhat work besides the found bugs.
Testing and code cleaning still required.

- [ ]  Batch Processing might not yet be reliable. A row or 2 might duplicate if a parameter is not right eg: Qmax or NoOfTerms.

- [ ] Dropdown menu Goes back to previous Item if user decides to change.

- [ ] All New Windows Are outside the SasView Interface.
…u. Additionally the dropdown menu used to not set to new selected data which has been fixed. however, in this version 2D data slicing is bracken this is due to some changes with how raw batch data is handles.
… the dataset. when switched back to the dateset (using the dropdown menu) the parameters selected prevously will be set.
…ead to a dir so each data file could be assigned to a thread instance. estimationThread and estimationThreadNT might also need to be changed as such. however more testing is required.
slicing: slices data are turned into model using the createModelItemWithPlot function.
this is makes it much easier to move around as it can work just like 1D data.

1D batch: Parameters set before batch processing is remembered and used when processed using batch mode.
…imations".

Parameters whilst, Batch Processing is also remembered.
Estimated data doesn't mess with actual data unless suggested button is pressed.
However, Duplication issue still remains.
…ne using to Calculate all button is still a bit buggy.
Still a bit more tweaking is required for batch processing slices.
…D rawImage and shows slice using phi. Fixed buttons. Minor cleanups. fixed 2D slices from not being batched processed.
for Devs: Plots in Pr can be disabled using self._allowPlots

Plot Behaviour:
if Batch or Data2D Slices -> hide Plots
if Button(Calculate) or Single Data1D -> show Plots
… the lager functions into smaller functions. And Changes the Warnings for GridPanel.py.
@lucas-wilkins
Copy link
Contributor

@krzywon should all be fixed.

@wpotrzebowski wpotrzebowski added the Discuss At The Call Issues to be discussed at the fortnightly call label Jul 29, 2024
@wpotrzebowski wpotrzebowski removed the Discuss At The Call Issues to be discussed at the fortnightly call label Aug 5, 2024
@wpotrzebowski wpotrzebowski added Discuss At The Call Issues to be discussed at the fortnightly call and removed Discuss At The Call Issues to be discussed at the fortnightly call labels Aug 12, 2024
@wpotrzebowski wpotrzebowski removed the SasView 6.0.0 Required for 6.0.0 release label Aug 27, 2024
krzywon pushed a commit that referenced this pull request Oct 25, 2024
… seem get to run with the following code

```
        GuiUtils.updateModelItemWithPlot(item, new_plot, new_plot.id)

        self.base.manager.communicator.plotUpdateSignal.emit([new_plot])
        self.base.manager.communicator.forcePlotDisplaySignal.emit([item, new_plot])
```

changes made using feedback from:

1) #2181 (comment)
removed as it was something that I was testing out but didn't end up using.

2) #2181 (comment)
Moved closeEvent() from InversionWidget.py to InversionPerspective.py.

3) #2181 (comment)
changed `from sas.sascalc.dataloader.manipulations import SectorQ` to `from sasdata.data_util.manipulations import SectorQ`
krzywon pushed a commit that referenced this pull request Oct 25, 2024
… seem get to run with the following code

```
        GuiUtils.updateModelItemWithPlot(item, new_plot, new_plot.id)

        self.base.manager.communicator.plotUpdateSignal.emit([new_plot])
        self.base.manager.communicator.forcePlotDisplaySignal.emit([item, new_plot])
```

changes made using feedback from:

1) #2181 (comment)
removed as it was something that I was testing out but didn't end up using.

2) #2181 (comment)
Moved closeEvent() from InversionWidget.py to InversionPerspective.py.

3) #2181 (comment)
changed `from sas.sascalc.dataloader.manipulations import SectorQ` to `from sasdata.data_util.manipulations import SectorQ`
@krzywon
Copy link
Contributor

krzywon commented Oct 25, 2024

This might be superseded by #3137. Keeping this open for now, but likely will be closed next week.

@rozyczko
Copy link
Member

One issue (?) found with the range sliders: moving them on the I(q) chart doesn't update the values in the Options tab for Total Q range. The other way - when modified in the Max:` textbox, the range change propagates to the chart

@rozyczko
Copy link
Member

Another: the widget should not be resizable. There is nothing to be gained by resizing. It only looks bizarre when you maximize this widget.
It's different than the Fitting Perspective, where resizing can help access and view parameters, when the table is large.

Copy link
Member

@rozyczko rozyczko left a comment

Choose a reason for hiding this comment

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

small changes requested for consideration

@@ -616,8 +616,22 @@ def updatePerspectiveWithProperties(self, key, value):
if 'pr_params' in value:
self.cbFitting.setCurrentIndex(self.cbFitting.findText('Inversion'))
params = value['pr_params']
self.sendItemToPerspective(items[0])
self._perspective().updateFromParameters(params)
# Make the perspective read the rest of the read data
Copy link
Member

Choose a reason for hiding this comment

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

this conditional contains the same content as the branch above (for fitting) and the common code should be merged

return True

@property
def supports_fitting(self):
Copy link
Member

Choose a reason for hiding this comment

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

this should probably be supports_fitting_menu

Copy link
Contributor

Choose a reason for hiding this comment

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

The default for supports_fitting_menu is False, so this might be unneccessary.

def communicator(self):
return self.communicate
return self.filesWidget.communicate
Copy link
Member

Choose a reason for hiding this comment

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

there's only one instance of Communicate - created in GuiManager. Both self.communicate (from the parent, which is GuiManager) or from filesWidget should be the same object. Why change it here?

key = table.horizontalHeaderItem(column).data(0)
for row in range(table.rowCount()):
Copy link
Member

Choose a reason for hiding this comment

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

avoid lazy try..except.

        for column in range(table.columnCount()):
            value = []
            key = table.horizontalHeaderItem(column).data(0)
            for row in range(table.rowCount()):
                item = table.item(row, column)
                if item is not None:
                    value.append(item.data(0))
                else:
                    value.append(" ")

Copy link
Contributor

@krzywon krzywon left a comment

Choose a reason for hiding this comment

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

Code wise, this is getting closer to finished.

Functionality wise, I'm seeing the following:

  • the save state is still broken - attempting to save with two tabs throws an error
  • the regularization constant and number of terms never seems to be calculated. Maybe it is slow, but that needs to get fixed.
  • Some of the sector slicer changes are still present. Updating the slicer parameter window updates the view, but not the 1D data. The 2D P(r), which is now disabled, should really be added to the batch slicer window as a fitting option.

@dehoni - Do you need/want help with any of this? I should be able to find time early next calendar year to work on this.

Comment on lines +458 to 462
if norm == 0:
ret_val = 0
else:
ret_val = np.sqrt(oscill/norm) / np.pi * self.d_max
return ret_val
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if norm == 0:
ret_val = 0
else:
ret_val = np.sqrt(oscill/norm) / np.pi * self.d_max
return ret_val
return 0 if norm == 0 else np.sqrt(oscill/norm) / np.pi * self.d_max

@@ -161,21 +161,26 @@ def __setattr__(self, name, value):
return self.set_y(value)
elif name == 'err':
value2 = abs(value)
if None in value:
Copy link
Contributor

Choose a reason for hiding this comment

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

If None is in the array, abs() will throw a TypeError and this message will not be reachable.

Comment on lines +338 to +341
if total == 0:
return 0
else:
return total_pos / total # dx cancels
Copy link
Contributor

Choose a reason for hiding this comment

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

I have seen this same paradigm done in three different ways. They could all be simplified to return 0 if a == 0 else x / a

(A)

if a == 0:
    return 0
else:
    return x / a

(B)

if a == 0:
    return 0
return x / a

(C)

if a == 0:
    value = 0
else:
    value = x / a
return value

Comment on lines +65 to +68
# if output_data is not None:
# # Set a table tooltip describing the model
# model_name = output_data[0][0].model.id
# self.tabWidget.setTabToolTip(0, model_name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Removing unused code doesn't hurt


# Default Values for inputs
NUMBER_OF_TERMS = 10
REGULARIZATION = 0.0
Copy link
Contributor

Choose a reason for hiding this comment

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

The regularization constant has been 0.0001 in the past. Is there a reason for the change?

Comment on lines +191 to +194
def _calculateUpdate(self, params: dict, tab_name):
""" Update the perspective using a dictionary of parameters
e.g. those loaded via open project or open analysis menu items"""
raise NotImplementedError("Update from parameters not implemented yet.")
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this and why is it here?

return True

@property
def supports_fitting(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

The default for supports_fitting_menu is False, so this might be unneccessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocker Prevents a different issue from being resolved Inversion Perspective Concerns inversion perspective
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Functionality to export a report of the data once calculated Enable batch processing for P(r)
7 participants