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

2107 idf plot selection #2145

Merged
merged 25 commits into from
Aug 19, 2022
Merged

2107 idf plot selection #2145

merged 25 commits into from
Aug 19, 2022

Conversation

lucas-wilkins
Copy link
Contributor

@lucas-wilkins lucas-wilkins commented Aug 15, 2022

Goal: add the IDF plot as a tab

  • Added IDF tab to corfunc
  • Further refactored corfunc plotting
  • Added IDF to reports

@lucas-wilkins lucas-wilkins linked an issue Aug 15, 2022 that may be closed by this pull request
@butlerpd butlerpd added the Discuss At The Call Issues to be discussed at the fortnightly call label Aug 15, 2022
Copy link
Contributor

@wpotrzebowski wpotrzebowski left a comment

Choose a reason for hiding this comment

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

I have problems running it from the installer on Mac.

Traceback (most recent call last):
File "sasview.py", line 15, in
File "sas/qtgui/MainWindow/MainWindow.py", line 113, in run_sasview
File "sas/qtgui/MainWindow/MainWindow.py", line 44, in init
File "PyInstaller/loader/pyimod02_importers.py", line 493, in exec_module
File "sas/qtgui/MainWindow/GuiManager.py", line 35, in
ModuleNotFoundError: No module named 'sas.qtgui.Utilities.Reports'
[93029] Failed to execute script 'sasview' due to unhandled exception: No module named 'sas.qtgui.Utilities.Reports'
[93029] Traceback:
Traceback (most recent call last):
File "sasview.py", line 15, in
File "sas/qtgui/MainWindow/MainWindow.py", line 113, in run_sasview
File "sas/qtgui/MainWindow/MainWindow.py", line 44, in init
File "PyInstaller/loader/pyimod02_importers.py", line 493, in exec_module
File "sas/qtgui/MainWindow/GuiManager.py", line 35, in
ModuleNotFoundError: No module named 'sas.qtgui.Utilities.Reports'

Does it work on Windows?
I guess there maybe some path missing in pyinstaller file.

@lucas-wilkins
Copy link
Contributor Author

@wpotrzebowski I think @krzywon has fixed this in the as-yet-unmerged #2139

@lucas-wilkins
Copy link
Contributor Author

@wpotrzebowski Try it now?

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.

Looks good, minor issues raised

src/sas/qtgui/Perspectives/Corfunc/CorfuncCanvas.py Outdated Show resolved Hide resolved
src/sas/qtgui/Perspectives/Corfunc/CorfuncCanvas.py Outdated Show resolved Hide resolved
src/sas/qtgui/Perspectives/Corfunc/CorfuncPerspective.py Outdated Show resolved Hide resolved
Comment on lines 26 to 27
if self.data is not None:
if len(self.data) == 2:
Copy link
Member

Choose a reason for hiding this comment

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

can be squeezed into one line

Copy link
Contributor

Choose a reason for hiding this comment

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

Another comment on these two lines -> typing only gives a suggestion of what should be passed to a method, but isn't a requirement. If self.data is a string two characters long, what happens?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its a suggestion in the same way that a police officer might suggest that you've had too much to drink and that its time to go home.

src/sas/sascalc/data_util/calcthread.py Show resolved Hide resolved
@wpotrzebowski
Copy link
Contributor

Indeed, it seems to work now (from installer)

@wpotrzebowski
Copy link
Contributor

I've tried it on two files that I think it used to work before (see attached screen shot).
Also Corfunc window on laptop screen is too big (and cannot be minimized to fit available space).
Screenshot 2022-08-16 at 11 31 45

11:25:29 - INFO: --- SasView session started, version 5.0.5, 2022 --- 11:25:29 - INFO: Python: 3.8.13 (default, Jul 22 2022, 10:40:33) [Clang 12.0.0 (clang-1200.0.32.29)] 11:30:49 - ERROR: 'list' object has no attribute 'x' Traceback (most recent call last): File "sas/qtgui/MainWindow/DataExplorer.py", line 785, in sendData File "sas/qtgui/Perspectives/Corfunc/CorfuncPerspective.py", line 469, in setData File "sas/qtgui/Perspectives/Corfunc/CorfuncCanvas.py", line 63, in data File "sas/qtgui/Perspectives/Corfunc/QSpaceCanvas.py", line 78, in draw_data AttributeError: 'list' object has no attribute 'x' 11:31:22 - ERROR: 'method' object is not connected Traceback (most recent call last): File "sas/qtgui/MainWindow/DataExplorer.py", line 785, in sendData File "sas/qtgui/Perspectives/Corfunc/CorfuncPerspective.py", line 454, in setData TypeError: 'method' object is not connected

@rozyczko
Copy link
Member

Same behaviour on Windows (on sending a file to the corfunc perspective)

Traceback (most recent call last):
  File "sas\qtgui\MainWindow\DataExplorer.py", line 785, in sendData
  File "sas\qtgui\Perspectives\Corfunc\CorfuncPerspective.py", line 469, in setData
  File "sas\qtgui\Perspectives\Corfunc\CorfuncCanvas.py", line 63, in data
  File "sas\qtgui\Perspectives\Corfunc\QSpaceCanvas.py", line 78, in draw_data
AttributeError: 'list' object has no attribute 'x'

@lucas-wilkins
Copy link
Contributor Author

@rozyczko @wpotrzebowski Ah, this was broken by the merge from main to fix the installer.

Copy link
Contributor

@smk78 smk78 left a comment

Choose a reason for hiding this comment

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

Functionality test on W10/x64.
Does what it advertises. Just one minor bug: the legend for the IDF does not appear.
image
The Log Explorer says:

 WARNING: No handles with labels found to put in legend.

@rozyczko
Copy link
Member

@rozyczko @wpotrzebowski Ah, this was broken by the merge from main to fix the installer.

and it works now!

@lucas-wilkins lucas-wilkins requested a review from krzywon August 17, 2022 15:36
@lucas-wilkins
Copy link
Contributor Author

@wpotrzebowski @smk78 Better?

@wpotrzebowski
Copy link
Contributor

Window scalling works fine now.

Two potential issues that arised while testing

  1. Label and legend on IDF plot missing (as mentioned on the call on Tuesday)
  2. When trying to generate report I get the following error:
8:52:20 - INFO:  --- SasView session started, version 5.0.5, 2022 ---
08:52:20 - INFO: Python: 3.8.13 (default, Jul 22 2022, 10:40:33) 
[Clang 12.0.0 (clang-1200.0.32.29)]
08:52:47 - WARNING: Background value results in negative intensities (1.051 > 0.44883)
08:54:14 - ERROR: Traceback (most recent call last):
  File "sas/qtgui/MainWindow/GuiManager.py", line 840, in actionReport
  File "sas/qtgui/Perspectives/Corfunc/CorfuncPerspective.py", line 788, in getReport
  File "sas/qtgui/Utilities/Reports/reports.py", line 106, in __init__
  File "importlib/resources.py", line 169, in read_text
  File "importlib/resources.py", line 125, in open_text
  File "PyInstaller/loader/pyimod02_importers.py", line 539, in open_resource
  File "pathlib.py", line 1222, in open
  File "pathlib.py", line 1078, in _opener
FileNotFoundError: [Errno 2] No such file or directory: '/private/var/folders/q3/5vy4nct93wg03ynnn836g49m0000gq/T/AppTranslocation/58FB7774-5D37-4038-9D15-6BD37D4AB3DB/d/SasView5.app/Contents/MacOS/sas/qtgui/Utilities/Reports/report_style.css' 

@lucas-wilkins
Copy link
Contributor Author

Label and legend on IDF plot missing (as mentioned on the call on Tuesday)

Legend was deleted, as there's only one curve on the IDF plot.

Axis label is not missing, it's just a long way to the left - it shows on my screen, which is perhaps wider than yours - if you have a sensible fix for it let me know.

When trying to generate report I get the following error:

Ooer, I'll look at that,, think its probably about packaging the CSS. I thought @krzywon's fix should have sorted that.

@rozyczko
Copy link
Member

When trying to generate report I get the following error:

Ooer, I'll look at that,, think its probably about packaging the CSS. I thought @krzywon's fix should have sorted that.

I think the CSS file needs to be put somewhere more central than
SasView\\sas\\qtgui\\Utilities\\Reports\\report_style.css
We probably don't want to recreate this path in the installed app

Copy link
Contributor

@smk78 smk78 left a comment

Choose a reason for hiding this comment

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

Functionality test on W10/x64 using a 1920x1080 monitor.
When I first brought up the Corfunc perspective the bottom of the perspective was somewhere in the midst of the Lamellar Parameters dialog and I had to stretch the perspective downwards, which I was a little surprised about as there was plenty of screen real estate.
I note the (missing) IDF legend has now been removed completely. Which is fine. But as others have noted, the y-axis label is not initially displaying correctly:
image
However, stretching the perspective window to the right fixes this.
Saving extrapolated & transformed data to file worked fine.
Generating a report worked fine, and looked fine in the report dialog window, but the actual output is real ugly and needs looking at.
corfunc_report.pdf
At some point in what I was doing this appeared in the Console, possibly when I was 'reporting':

WARNING: getSize: Not a float '100%'

@lucas-wilkins
Copy link
Contributor Author

When I first brought up the Corfunc perspective the bottom of the perspective was somewhere in the midst of the Lamellar Parameters dialog and I had to stretch the perspective downwards, which I was a little surprised about as there was plenty of screen real estate.

Yeah, it's either that or no scrolling. Unless you can think of a way of coercing the window to a good size.

@lucas-wilkins
Copy link
Contributor Author

CSS file should now be included in the installers

@lucas-wilkins
Copy link
Contributor Author

WARNING: getSize: Not a float '100%'

That's the html to pdf converter not liking how the width of the images is specified.

Copy link
Contributor Author

@lucas-wilkins lucas-wilkins left a comment

Choose a reason for hiding this comment

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

@wpotrzebowski this should fix the report issue you had

Copy link
Contributor

@smk78 smk78 left a comment

Choose a reason for hiding this comment

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

Testing latest changes on W10/x64. Plots are now separated in the report.

@lucas-wilkins
Copy link
Contributor Author

SasView can find the css file now, but doesn't have permission to read it for some reason.

@lucas-wilkins
Copy link
Contributor Author

Woooooo, finally, reports bugs all squashed!

@lucas-wilkins lucas-wilkins merged commit 8514803 into main Aug 19, 2022
@krzywon krzywon deleted the 2107-idf-plot-selection branch August 19, 2022 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IDF plot selection
6 participants