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

Add updated tutorial: Simultaneous-1D-Fitting #15

Merged
merged 7 commits into from
Oct 15, 2024

Conversation

smk78
Copy link
Collaborator

@smk78 smk78 commented Jan 22, 2024

This PR provides an updated version of the Simultaneous-1D-Fitting-in-SasView-v5 tutorial.

Addresses #1
Addresses #3

  • All screenshots have been regenerated due to GUI changes
  • Additional screenshots have been incorporated to show functionality added to the GUI
  • Fitting has been completely re-done and is now giving about the same results as the v4.x tutorial
  • Significant text/formatting changes throughout

When merged, the .pdf file will then need moving to Sasview/sasview/src/sas/sasview/media in an appropriate branch, and copying to Sasview/sasview.github.io/downloads. It is provided here simply as a convenience for the reviewer(s).

@butlerpd
Copy link
Member

butlerpd commented Feb 4, 2024

Thanks for the pdf @smk78! version 5.0.6 looks pretty good (I have not looked at v6.0.0). I have the following questions/comments:

  • in the glossary table, you refer to "compute (button)." But later in the tutorial you refer to it as compute/plot button (which I think is what it is). I realize you are not talking about the plot part of compute/plot but wonder if that will be confusing?
  • you point out that all the files should be checked in the Data Explorer if you just loaded them all together but you also point out what to do if a person got to that point in the tutorial from a different path. However you just say hit the send button without mentioning that the default "fitting" option needs to be in the dropdown box. That will be true if they have not played around with stuff before starting the tutorial, but it might be good to add a tiny note to that effect as you did for the files being checked?
  • I am confused on the polydispersity.
    • you specifically state that we cannot fit polydispersity in a constrained fit in 5.0.6 but will in 6.0.0. However you then proceed to have us fit said polydispersity? I was totally confused by that section I guess
    • Actually I'm still fuzzy on the constrained fits with polydispersity but I thought the issue was a warning about fitting any polydisperse parameter, not just the polydispersity on the parameter? The issue was identified and explained by @RichardHeenan who may be able to clarify? Not sure how deep we really want to go into that? But I have just been fixing the polydispersity on the parameters and never fitting them but do fit the polydisperse parameter itself (such as radius etc)
    • I know @gonzalezma was working on cleaning up all the constrained fitting issues but has this issue been fixed in 6.0.0? I had not realized that. Would be great if it is but wanted to make sure before saying so. Also I guess that means the warning should be removed when doing constrained fits.

@butlerpd
Copy link
Member

And now for the version 6 document.

  • page 3, the orange note still says that his is for version 5.0.6. This should now read 6.0.0
  • Otherwise the previous comments hold. In particular regarding whether polydisperse constraints are really completely fixed by @gonzalezma.

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.

there is one minor error as noted in the comments. A couple of other questions which may or may not require changes.

@smk78 smk78 self-assigned this Feb 13, 2024
@gonzalezma
Copy link

In addition to Paul's remark about correcting 5.6.0 to 6.0.0 in the v6 tutorial, I would propose also to remove the sentence "In this example we shall use the 'shell' contrast theory (M3) as the 'parent' theory in the expectation that it has greater sensitivity to the shell thickness, but this is an arbitrary choice". IMO, one defines a set of constraints and sends a single global problem to bumps, so the choice of the parent theory is irrelevant. All should converge to the same result.

Concerning Paul's questions about the polydisperse constraints:

  • Constraints on PD parameters work on 6.0.0, as shown in the v6 tutorial (and I just re-tested in the current release branch).
  • Constraints on PD parameters are not available in 5.0.6 (they will not appear in the menu), but you can of course fit the PD parameters (without the constraints). This is what the tutorial for 5.0.6 shows: The values for radius and thickness distributions obtained for each of the 3 data sets are different.
  • The warning about applying constraints to a PD parameter is independent of the fact that the distribution width is either fixed, fitted or constraints between widths are applied. This warning was added because a user fitting e.g a cylinder could define a constraint length = radius. This constraint will be applied and the results will indeed show length = radius. But the real calculation of I(q) proceeds in 2 independent loops. So even if the two distributions are exactly the same (equal value and width), for each single value of the length distribution we go through the whole radius distribution (so that length != radius). The only way of fixing this is reparameterizing the model. In any case, this does not apply to the present tutorial (and I did not see this mentioned in the tutorial, or did I miss it?). However, the warning appearing in the Complex Constraint window is still relevant, although perhaps it is not very informative for a non-expert (or even an expert) user. But I found hard to explain the problem in a few words. A possible alternative could be to keep only the red Warning message and add a 'More info' button that could point to the relevant documentation page (and if needed write/extend this).

@smk78
Copy link
Collaborator Author

smk78 commented Mar 28, 2024

@Butler , @gonzalezma , thank you for your comments.

I have made substantive edits to both the v5 and particularly v6 documents.

  • the Glossary now refers to the Compute/Plot button :-) (and in the basic-1d-fitting tutorials too)
  • the v6 tutorial now correctly refers to v6.0.0!
  • the v6 tutorial now shows screenshots from v6!
  • I added some text so the instructions now say "ensure that the analysis drop-down box says Fitting and click the Send To button."
  • as @gonzalezma says, whilst you can fit the radius/thickness polydispersities in both v5 and v6, you can only use those polydispersities in constraints in v6. I've added more text to both tutorials to try and make that even clearer.
  • @gonzalezma has, I think, addressed the comment @butlerpd attributes (I think correctly) to @RichardHeenan about fitting polydisperse parameters. But that is outside of the scope of these tutorials.
  • I have removed the sentence about using the "shell" theory as a parent theory.

@smk78
Copy link
Collaborator Author

smk78 commented Jul 24, 2024

Have updated both versions of this tutorial for the minor stylistic comments made in #14 (comment)

@wpotrzebowski
Copy link
Contributor

I might have missed it but I think it it is a first time we release with weighting scheme on simultaneous fitting (5th column).
Screenshot 2024-08-01 at 15 53 20

I know @gonzalezma worked on a separate tutorial for it. I guess there is no point in merging this tutorial with the work of @gonzalezma but I think it is at least worth mentioning it.

@smk78
Copy link
Collaborator Author

smk78 commented Aug 21, 2024

I might have missed it but I think it it is a first time we release with weighting scheme on simultaneous fitting (5th column). !

I know @gonzalezma worked on a separate tutorial for it. I guess there is no point in merging this tutorial with the work of @gonzalezma but I think it is at least worth mentioning it.

So interestingly, that functionality is not in the 6.0.0 beta2 Windows installer version. Is it in the Mac version? Or is it not yet added to the release branch?

@butlerpd
Copy link
Member

oh oh .... just saw this. My head is spinning from testing and testing and reporting all ready, but we need to look into this. It was merged a long time ago was my recollection and understanding? For another day though. I'm "burnt to a crisp"

@smk78
Copy link
Collaborator Author

smk78 commented Aug 22, 2024

oh oh .... just saw this. My head is spinning from testing and testing and reporting all ready, but we need to look into this. It was merged a long time ago was my recollection and understanding? For another day though. I'm "burnt to a crisp"

Merged to main in Apr 26 2022 according to SasView/sasview#1973
If I interpret the commit history correctly, the first commit to the release_6.0.0 branch was Apr 11 (ie, before the merge).
I also note that PR1973 is not in the 6.0.0-alpha changelog.
Maybe we decided not to cherry-pick it into v6?

@smk78
Copy link
Collaborator Author

smk78 commented Aug 22, 2024

So interestingly, that functionality is not in the 6.0.0 beta2 Windows installer version. Is it in the Mac version? Or is it not yet added to the release branch?

CORRECTION: @krzywon points out you have to check the "Modify weighting" box before the column appears.

image

@smk78
Copy link
Collaborator Author

smk78 commented Aug 22, 2024

The v5 & v6 tutorials now carry a Forward explaining when you might/not want to use simulataneous fitting and some potential traps. The v6 document now says how to invoke the weighting column and refers the reader to @gonzalezma 's tutorial on using @Caddy-Jones 's functionality.

I took the liberty of adding a SasView6 branding to the front of @gonzalezma 's tutorial!

@smk78
Copy link
Collaborator Author

smk78 commented Oct 15, 2024

Agreed at Leadership Team meeting 15/10/24 to merge this without further review.

@smk78 smk78 merged commit 6efe3f1 into master Oct 15, 2024
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.

4 participants