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

Sasview counterpart to sasdata manipulations rewrite #2615

Conversation

ehewins
Copy link
Contributor

@ehewins ehewins commented Sep 17, 2023

Description

This pull request, as the title suggests, is the sasview counterpart to my rewrite of sasdata's manipulations.py. Full details are given in this pull request.

Fixes #2593

Review Checklist (please remove items if they don't apply):

  • Code has been reviewed
  • Functionality has been tested
  • Windows installer (GH artifact) has been tested (installed and worked)
  • MacOSX installer (GH artifact) has been tested (installed and worked)
  • User documentation is available and complete (if required)
  • Developers documentation is available and complete (if required)
  • The introduced changes comply with SasView license (BSD 3-Clause)

…t, and changed variable names to new versions. Also removed the angle transformation, as new_manipulations uses the same -pi to pi interval as the slicers.
Copy link
Contributor

@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.

Should be fine if the sasdata bit is

@wpotrzebowski wpotrzebowski added the Discuss At The Call Issues to be discussed at the fortnightly call label Sep 25, 2023
@wpotrzebowski wpotrzebowski changed the base branch from main to release_6.0.0 September 26, 2023 13:27
@wpotrzebowski wpotrzebowski removed the Discuss At The Call Issues to be discussed at the fortnightly call label Sep 26, 2023
@krzywon
Copy link
Contributor

krzywon commented Sep 26, 2023

Everything here looks fine, but I have a few suggestions.

  1. If sasdata.data_util.manipulations.py is no longer used, replace deprecate it.
  2. To test the SasView build using unmerged changes in dependent repos, modify .github/workflows/ci.yml, replacing master in git clone --depth=50 --branch=master https://github.com/SasView/<repo>.git ../<repo> with the name of your branch, but be sure to revert back before merging.

@wpotrzebowski wpotrzebowski added the Discuss At The Call Issues to be discussed at the fortnightly call label Oct 9, 2023
@wpotrzebowski
Copy link
Contributor

@ehewins will you be willing to take a look on @krzywon suggestions? No worries if not - we need to know how to move forward with it.

@ehewins
Copy link
Contributor Author

ehewins commented Oct 9, 2023

Hi @wpotrzebowski , apologies about the lack of activity recently. I wish I could, but university work has completely taken over my life at the moment. I had wanted to do some more work on my unit test file in the other repo, and was up for doing this too, but I don't think I'll have time until several weeks from now.
Realistically I think your best option is to assume I won't be doing any more work on the area. I might surprise you all with a commit or two about a month from now though (if I can organise my time well enough).

@wpotrzebowski
Copy link
Contributor

@krzywon will take a quick look at it

@wpotrzebowski wpotrzebowski removed the Discuss At The Call Issues to be discussed at the fortnightly call label Oct 10, 2023
@krzywon
Copy link
Contributor

krzywon commented Oct 17, 2023

I have a commit to push here, but want to be sure the changes I made in SasView/sasdata#47 are acceptable, so I don't pollute two different PRs. The changes have to do with the file name change (from maniuplations_new.py to averaging.py).

@butlerpd butlerpd added the Discuss At The Call Issues to be discussed at the fortnightly call label Oct 23, 2023
Copy link
Contributor

@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.

Needs to be updated to refer to averaging, not new_manipulations

src/sas/qtgui/Plotting/Plotter2D.py Outdated Show resolved Hide resolved
src/sas/qtgui/Plotting/Slicers/SectorSlicer.py Outdated Show resolved Hide resolved
@butlerpd butlerpd removed the Discuss At The Call Issues to be discussed at the fortnightly call label Oct 24, 2023
@krzywon
Copy link
Contributor

krzywon commented Oct 26, 2023

This PR and the associated one, SasView/sasdata#47, are ready for a full review. The CI for this PR points to the associated sasdata branch and should be changed prior to merging.

@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 Nov 7, 2023
@butlerpd butlerpd added the Discuss At The Call Issues to be discussed at the fortnightly call label Nov 19, 2023
@wpotrzebowski wpotrzebowski removed the Discuss At The Call Issues to be discussed at the fortnightly call label Dec 3, 2023
@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 Dec 4, 2023
@jack-rooks
Copy link
Contributor

jack-rooks commented Jan 18, 2024

Using the requirements specified by this branch (following the setup guide) results in a problem for matplotlib 3.5.3 (matplotlib/matplotlib#24374)

Can be fixed by following the solution there (installing matplotlib 3.6.2). Assuming I followed the correct install procedure it may be necessary to update build_tools/requirements.txt to have matplotlib==3.6.2)

Note, this is not a problem for the built installer as it uses the appropriate version

@lucas-wilkins
Copy link
Contributor

Closing because too out of sync, not deleting branch though

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.

Neither fold=True nor fold=False is appropriate for WedgeSlicer
6 participants