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

fixed xaxis label for updated SESANS nomenclature from z to delta #2754

Merged
merged 10 commits into from
Mar 20, 2024

Conversation

caitwolf
Copy link
Contributor

Description

Changes the x-axis label on the residuals SESANS plots from z to delta.

Fixes #2663

How Has This Been Tested?

Load SESANS example data, send to fitting, and plot residuals. This PR is tied to another in sasdata that fixes the x-axis label on the 1D data plot for SESANS (SasView/sasdata#60).

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)

@caitwolf caitwolf linked an issue Jan 19, 2024 that may be closed by this pull request
@caitwolf caitwolf marked this pull request as draft January 19, 2024 18:07
@caitwolf caitwolf requested a review from krzywon January 22, 2024 19:08
@caitwolf caitwolf marked this pull request as ready for review January 22, 2024 19:12
Copy link
Member

@butlerpd butlerpd left a comment

Choose a reason for hiding this comment

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

Not sure that it was necessary to create a new shortcut for delta rather than just using the latex, but there is mixed usage already so guess it is OK

spin Echo length label is now generated for the plot .. but not the residual. Otherwise looks good

@caitwolf
Copy link
Contributor Author

The other plot should be updated when the changes in SasView/sasdata#60 are implemented

@caitwolf
Copy link
Contributor Author

@butlerpd did you have a chance to try the new installer that includes the edits from the paired sasdata branch and PR?

@caitwolf caitwolf requested a review from butlerpd January 30, 2024 18:53
@butlerpd
Copy link
Member

Also thinking this should be merged also into 6.0.0 for the beta release?

@caitwolf
Copy link
Contributor Author

caitwolf commented Feb 12, 2024

I agree about putting this into 6.0.0, as well as the paired PR #60 in sasdata. The last steps to be completed are:

  • switch the target branch from main to 6.0.0 - or cherry-pick commits over once merged? I would need some advice on the correct path forward
  • test with the updated installer that includes changes on the sasdata side
  • fix the ci.yml file back to the correct sasdata branch before merging

@caitwolf caitwolf changed the base branch from main to release_6.0.0 February 12, 2024 20:21
@caitwolf caitwolf changed the base branch from release_6.0.0 to main February 12, 2024 20:22
Copy link
Member

@butlerpd butlerpd left a comment

Choose a reason for hiding this comment

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

I understand that the choice of using the unicode or defining the symbol in rst_prolog was based on how other symbols were used in each file. I still think this is confusing and it would be nice to refactor everything for consistency but that is certainly beyond the scope of this PR.

Otherwise the code looks good both here and the paired PR in sasdata and does what it should do on windows.

SOME CAVEATS:

  • Given this is about plotting greek symbols it may be wise to check that it behaves as expected on MacOS
  • More importantly, changes to the ci.yml file should be reversed PRIOR to the merge
  • The paired PR should be merged at the same time
  • This should definitely go into 6.0.0.

@butlerpd butlerpd added the Discuss At The Call Issues to be discussed at the fortnightly call label Feb 13, 2024
@wpotrzebowski
Copy link
Contributor

Looks fine on MAC

@butlerpd butlerpd removed the Discuss At The Call Issues to be discussed at the fortnightly call label Feb 13, 2024
@butlerpd butlerpd changed the base branch from main to release_6.0.0 February 27, 2024 20:26
@butlerpd butlerpd changed the base branch from release_6.0.0 to main February 27, 2024 20:27
@butlerpd
Copy link
Member

Oops ... NOT ready to merge to 6.0.0 ... and will not be ready once PR #2808 is merged into main.

Basically data_formats_help.rst has moved from sasview to sasdata. Thus the changes to sasview/src/sas/qtgui/MainWindw/media/data_formats_help.rst should be removed and added back to sasdata/docs/source/user/data/data_formats.rst. I would recommend reverting the ci.yml change also at this point to make the rebasing clean and easy.

Otherwise rebasing works and this will be able to be rebased onto release_6.0.0 without problem

Copy link
Member

@butlerpd butlerpd left a comment

Choose a reason for hiding this comment

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

see most recent comment needing fixing before this can be merged

@caitwolf caitwolf changed the base branch from main to release_6.0.0 March 12, 2024 12:35
@caitwolf caitwolf changed the base branch from release_6.0.0 to main March 12, 2024 12:45
@caitwolf caitwolf changed the base branch from main to release_6.0.0 March 12, 2024 12:55
@caitwolf caitwolf requested a review from butlerpd March 12, 2024 13:15
@caitwolf caitwolf changed the base branch from release_6.0.0 to main March 12, 2024 13:49
@caitwolf caitwolf force-pushed the 2663-update-nomenclature-for-sesans-plots branch from 341a38c to 4ae3ed5 Compare March 12, 2024 14:01
@butlerpd butlerpd force-pushed the 2663-update-nomenclature-for-sesans-plots branch from c889d20 to 6794a8f Compare March 12, 2024 17:38
@butlerpd butlerpd changed the base branch from main to release_6.0.0 March 12, 2024 17:39
@caitwolf
Copy link
Contributor Author

@butlerpd I updated the docs after the rebasing step to correctly display the new delta character and also show the updated sesans fitting screenshots

Copy link
Member

@butlerpd butlerpd left a comment

Choose a reason for hiding this comment

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

This looks pretty good now and is running nicely under release_6.0.0. I do have a couple of requests:

  • there is a missing \ symbol in from of a lambda in a math entry (simple fix)
  • Thanks for updating the plots in the fitting documentation. That too often gets forgotten! However, they have a very low resolution and hard to read. It would be good if we could get the same resolution as the other figures in that file.

@butlerpd
Copy link
Member

Also .. why are we bothering with changing the rst_prolog? It no longer is getting used right? If not, we should revert that to keep things clean.

@lucas-wilkins lucas-wilkins added the SasView 6.0.0 Required for 6.0.0 release label Mar 15, 2024
@butlerpd
Copy link
Member

  • there is a missing \ symbol in from of a lambda in a math entry (simple fix)

Right ... I forgot that is actually in the sasdata repo .... +1 to whoever said multiple repos are a pain 😄 - consider this a comment in sasdata instead please. Sorry about that.

@caitwolf caitwolf requested a review from butlerpd March 19, 2024 22:22
@caitwolf
Copy link
Contributor Author

@butlerpd Updated the "This help document was last changed by..." statement and last two sesans fitting images in the fitting help doc. PR is ready for review once the checks have completed.

Copy link
Member

@butlerpd butlerpd left a comment

Choose a reason for hiding this comment

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

The graphics are now much better and match the rest of the document.

I did notice two small things that would be nice to fix still - don't know why I did not catch before. For 6.0 the test folder has been replaced with the example_data folder. @smk78 has changed most of those but probably did not realize (or forgot -- I see he is the original author of that line) there was a reference to it in this document in the sesans section?

While we are at it the line about

In the screenshot above, the radius of the sphere has been increased from its default value of 50 Å to 5000 Å in order to get the transform to show something sensible.

can probably just be removed. I'm not sure why @smk78 wrote that unless he had a different example graphic he used? I think the previous sentence is sufficient. Alternatively it could be replaced with a line saying something like:

In the screenshot above for example, the radius of the sphere could be increased from its default value of 50 Å to 5000 Å in order to get the transform to show something more sensible.

@butlerpd butlerpd merged commit 8b6e4ee into release_6.0.0 Mar 20, 2024
36 checks passed
@butlerpd butlerpd deleted the 2663-update-nomenclature-for-sesans-plots branch March 20, 2024 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SasView 6.0.0 Required for 6.0.0 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update nomenclature for SESANS plots
5 participants