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

Updated rG&rg #2548

Closed
wants to merge 7 commits into from
Closed

Updated rG&rg #2548

wants to merge 7 commits into from

Conversation

smalex-z
Copy link
Contributor

@smalex-z smalex-z commented Jul 7, 2023

Description

Upon discussion with @butlerpd @pkienzle and @yunliu01, we decided it would be best to make the difference between Mass Based Radius of Gyration (Rg) and SLD Based Guinier Radius (RG) more obvious. I have separated the radius of gyration into two boxes, one for Mass-Based and one for SLD-Based.

I have also included code to change the SLD-Based RG based on the Solvent SLD, but that is commented out until a standard for determining protein volume can be agreed upon.

How Has This Been Tested?

Open Generic Scattering Calculator

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)

Copy link
Contributor

@krzywon krzywon left a comment

Choose a reason for hiding this comment

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

A couple of suggestions to clean up the code, but nominally does what it says it does. I can't speak to the accuracy, because no documentation was added for the calculation being performed.

@smalex-z smalex-z mentioned this pull request Jul 15, 2023
7 tasks
@butlerpd butlerpd added the Discuss At The Call Issues to be discussed at the fortnightly call label Jul 16, 2023
@butlerpd
Copy link
Member

This pull request and #2538 should be reviewed and hopefully approved but should d to wait be merged into one big pull request to resolve all the UI conflict and that pull request then merged. To avoid confusion I'm labeling this as WIP

@butlerpd butlerpd marked this pull request as draft July 19, 2023 16:29
@butlerpd butlerpd removed the Discuss At The Call Issues to be discussed at the fortnightly call label Jul 19, 2023
@smalex-z smalex-z mentioned this pull request Jul 24, 2023
7 tasks
@butlerpd butlerpd added the Discuss At The Call Issues to be discussed at the fortnightly call label Jul 25, 2023
@butlerpd
Copy link
Member

Once Jeff approves this we need to close the PR as the changes have been munged into PR #2565. This PR should NOT be merged

@pkienzle
Copy link
Contributor

These changes are mostly repeated in #2565. Review and merging will be easier if you combine them into one PR, or base one PR off the other.

@smalex-z
Copy link
Contributor Author

This PR and #2538 is not supposed to be merged- the changes are all repeated in #2565 because of merge conflicts with UI files. Once all 3 PRs are approved, only #2565 will be merged into main with the changes from all 3 PRs.

@krzywon krzywon closed this Aug 8, 2023
@butlerpd
Copy link
Member

@krzywon can this branch now be safely deleted? I think this is the other PR that was merged into a bigger one before closing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discuss At The Call Issues to be discussed at the fortnightly call
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants