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

2447 plot roles #2448

Merged
merged 13 commits into from
Mar 14, 2023
Merged

2447 plot roles #2448

merged 13 commits into from
Mar 14, 2023

Conversation

krzywon
Copy link
Contributor

@krzywon krzywon commented Feb 28, 2023

Description

A new plotting role is introduced here to flag specific plots as a stand-alone. This is separate from the residual role which has unique characteristics. Ths role allows a plot to be displayed on its own without the parent data set and has been applied to the P(r) and polydispersity plots so the residual lines added in #2443 are not shown.

Fixes #2447

How Has This Been Tested?

Run sasview. Run a fit with polydispersity. Residual plots have the +/- 1 and 3 lines, dispersity plots do not. Run a P(r) inversion. P(r) plots do not have the +/- lines.

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)
  • Developers documentation is available and complete (if required)
  • The introduced changes comply with SasView license (BSD 3-Clause)

@butlerpd
Copy link
Member

butlerpd commented Mar 1, 2023

I am not convinced that the two new roles: ROLE_STAND_ALONE_LINEAR and ROLE_STAND_ALONE_LOG are the right answer. I rather think the original commit of just a single ROLE_STAND_ALONE role would be appropriate.

Firstly, this is really log log and linear log (not linear linear as the name might imply). There can be log linear and linear linear. In fact, the polydispersity plots are linear linear for example. Furthermore, looking to the future, one probably wants to move the "change scale" from actually changing the data to x and y transforms. Finally, data1D objects already have a property that seems to do this Data1D.ytransform and Data1D.xtransform 2D data seems to have a similar thing but applies to the intensity I think so Data2D.scale? These are currently being used to plot the data on the appropriate scale (log or linear) so having a second way to do the same thing seems a bit ... dangerous? (spoken as someone who got bitten by competing ways to do the same thing before so that changing the default in the code did not actually change the default 😄 )

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.

Code looks good now. I will defer to others on whether the long term choice should be to leave the scale choice to the plottable object .. though the more I think about it the more that seems right.
Functionality is tested on windows and the fix does what it says. I recommend we merge post haste.

@rozyczko
Copy link
Member

rozyczko commented Mar 8, 2023

This looks good. the standalone role could be used as a starting point for refactoring of the "quick plots" which make the plotting subsystem more complex than it should be.

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

@butlerpd
Copy link
Member

do we need somebody to test on a mac or can we say that for this what has been done should be good enough and just merge?

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

Tested on Mac. Plots seem to work as expected. The only thing that I spotted is the lenged box on the plots is unnecessarily big. Not sure if this was introduced by this PR

Screenshot 2023-03-14 at 08 18 44
Screenshot 2023-03-14 at 08 19 45
Screenshot 2023-03-14 at 08 26 29

We also need to remember to document new features

@butlerpd butlerpd merged commit b956b61 into main Mar 14, 2023
@butlerpd butlerpd deleted the 2447-plot-roles branch March 14, 2023 15:02
@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.

Remove +/-1 and 3 sigma lines from P(r) plots
4 participants