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

Use sasdata package in place of sas.sascalc.dataloader #2141

Merged
merged 14 commits into from
Aug 30, 2022
Merged

Conversation

krzywon
Copy link
Contributor

@krzywon krzywon commented Aug 12, 2022

This PR replaces the sas.sascalc.dataloader subpackage with the stand-alone sasdata package. This is a step toward a complete separation between the calculation and GUI.

All data import/export capabilities, including unit tests, are now held within the sasdata package and are removed from sasview in this PR. Github workflows are updated to build and bundle the package into the installers.

This is a parallel PR to sasmodels 517. This PR also includes all work done in #2139.

@lucas-wilkins
Copy link
Contributor

I expect this will have quite a lot of collisions with #2145

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 (once I'd figured out how to test this PR!).

Tested loading individual and multiple (ctrl-click) 1D files of different types from 'Load data' button. OK.
Tested File > Load Data File(s) with 1D data. OK.
Tested File > Load Data Folder with 1D data. OK.
In some instances the following appeared in the Log Explorer:

WARNING: ascii_reader: could not load file

It would be more helpful if the filename that threw the warning was included.

Tested loading individual and multiple (ctrl-click) 2D files of different types from 'Load data' button. OK.
Tested File > Load Data File(s) with 2D data. OK.
Tested File > Load Data Folder with 2D data. OK.

Rejects coordinate and convertible files as it should with a sensible message in the Log Explorer. OK.

Also rejects image files (which should only load through the Image Viewer) but populates the Log Explorer with a whole lot of junk! It would be nice if this could be sanitised.

Image Viewer loads image files. OK.

Generic Scattering Calculator loaded coordinate files. OK.

Workspaces loaded in the Data Operation tool. OK.

So in functionality terms this PR seems fine (hence I'm approving), but a little tidying up on the messaging would be nice.

@butlerpd
Copy link
Member

Need to prioritize this PR due to the number of changes that might affect other work.

@lucas-wilkins
Copy link
Contributor

Seems to work with the pycharm workflow I have

@wpotrzebowski wpotrzebowski removed the Discuss At The Call Issues to be discussed at the fortnightly call label Aug 18, 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've been wondering where sasdata is included in the installer file and couldn't find it. Interestingly sasdata.__file__ run from the shell returns:
/Volumes/SasView5/SasView5.app/Contents/MacOS/sasdata/__init__.pyc, which doesn't exist.

It seems to work, so it is fine. It is just puzzling how it is imported and whether we need to anything for pyinstaller (like we did for sasmodels) to make sure is always running.

Otherwise looks good.

@rozyczko
Copy link
Member

Looks and behaves good on Windows.
python setup.py build install in sasdata properly installs the module.
All references in sasview seem to work correctly.
I guess python examples and jupyter notebooks in other repos will need updating?

@wpotrzebowski
Copy link
Contributor

Looks and behaves good on Windows. python setup.py build install in sasdata properly installs the module. All references in sasview seem to work correctly. I guess python examples and jupyter notebooks in other repos will need updating?

Yes, I was thinking the same but shouldn't that produce a folder/file in the installer? On Mac I would expect some trace of it in:

Screenshot 2022-08-18 at 12 45 22

@rozyczko
Copy link
Member

rozyczko commented Aug 18, 2022

Looks and behaves good on Windows. python setup.py build install in sasdata properly installs the module. All references in sasview seem to work correctly. I guess python examples and jupyter notebooks in other repos will need updating?

Yes, I was thinking the same but shouldn't that produce a folder/file in the installer? On Mac I would expect some trace of it in:

Screenshot 2022-08-18 at 12 45 22

Oh, you mean the installed binary. Then it probably got compiled in into the executable? All .py files should be compiled in, but sasmodels source is also explicitly added in the .spec file. sasdata is not

@krzywon
Copy link
Contributor Author

krzywon commented Aug 18, 2022

Should I add it to the spec? It wouldn't take much to make the change.

# Conflicts:
#	src/examples/test_panel2D.py
@wpotrzebowski
Copy link
Contributor

Should I add it to the spec? It wouldn't take much to make the change.

If the sasdata imports correctly I don't think it is necessary to add it explicetly to installer. I would even consider importing/installing sasmodels same way, so we can also remove it from installer. I don't really know why we need to add sasmodels and sasmodels-data explicetly. Maybe for docs?

@wpotrzebowski wpotrzebowski added the Discuss At The Call Issues to be discussed at the fortnightly call label Aug 29, 2022
@wpotrzebowski wpotrzebowski merged commit 3d56736 into main Aug 30, 2022
@wpotrzebowski wpotrzebowski deleted the use-sasdata branch August 30, 2022 14:00
@butlerpd butlerpd removed the Discuss At The Call Issues to be discussed at the fortnightly call label Aug 1, 2023
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.

6 participants