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

Doc Update: SESANS in Sasmodels #477

Merged
merged 6 commits into from
Oct 29, 2021
Merged

Doc Update: SESANS in Sasmodels #477

merged 6 commits into from
Oct 29, 2021

Conversation

smk78
Copy link
Contributor

@smk78 smk78 commented Oct 25, 2021

This is one of two PRs (the other for SasView) addressing SasView/sasview#1684 .
This PR should be merged first (since the SasView doc build copies down from Sasmodels).

The changes here are purely textual. A reference to Jurrian's paper (published in 2020) has been added, and the title of the page explaining how to use the functionality has been clarified. The latter is necessary because an equivalent page is added to the SasView docs by the second PR.

@smk78 smk78 requested a review from butlerpd October 25, 2021 18:21
@smk78 smk78 added this to the sasmodels 1.0.6 milestone Oct 25, 2021
Copy link
Contributor

@gonzalezma gonzalezma left a comment

Choose a reason for hiding this comment

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

In the change in sesans_fitting.rst, SESANS date should be SESANS data.

@smk78
Copy link
Contributor Author

smk78 commented Oct 27, 2021

Oops! Well spotted @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.

Looks good. Only two questions... or a question and a comment. Ok 2 questions and a comment

  1. I wonder if we should start adding DOIs (with external links perhaps?) to references? in which case the DOI: would be 10.3233/JNR-200154 and the link would be https://doi.org/10.3233/JNR-200154
  2. I think the line It is possible to fit SESANS data from the command line in Python is now utterly redundant and in facts looks very odd indeed placed where it is. In other words, in the section titled "fitting using the command line" and after the stuff about installing the developer environment. I can see how that was in there originally but suggest it be removed now
  3. Speaking of which, as another comment, it seems like this section should just be entirely replaced/deleted now and be part of the instructions on CLI use of SasView in general (using for example jupyter notebooks?

@wpotrzebowski
Copy link

sasmodels ready for testing on OSX

@smk78
Copy link
Contributor Author

smk78 commented Oct 28, 2021

In response to @Butler:

  1. I wonder if we should start adding DOIs (with external links perhaps?) to references? in which case the DOI: would be 10.3233/JNR-200154 and the link would be https://doi.org/10.3233/JNR-200154

Have added this DOI & link in sans_to_sesans.rst. Have also linked the existing DOI in sesans_fitting.rst.

I agree that adding DOIs throughout our help docs would be good, but it won't be a small task...

  1. I think the line It is possible to fit SESANS data from the command line in Python is now utterly redundant and in facts looks very odd indeed placed where it is. In other words, in the section titled "fitting using the command line" and after the stuff about installing the developer environment. I can see how that was in there originally but suggest it be removed now

Agreed. Now removed.

  1. Speaking of which, as another comment, it seems like this section should just be entirely replaced/deleted now and be part of the instructions on CLI use of SasView in general (using for example jupyter notebooks?

Remember that sans_to_sesans.rst & sesans_fitting.rst are part of Sasmodels which is agnostic in respect of Sasview. But also see SasView/sasview#1931.

@butlerpd
Copy link
Member

Have added this DOI & link in sans_to_sesans.rst. Have also linked the existing DOI in sesans_fitting.rst.

I agree that adding DOIs throughout our help docs would be good, but it won't be a small task...

Great! I agree - the bigger task is "no small task" indeed! My thinking was that if we agreed that DOI would be a good idea in this day and age (seems a no brainer?) we could start by asking for them to be included whenever a new reference is addedd and or an update of a document with some references is made? This will take time to get "full coverage of references" but at least gets us started and without more resources probably the best we can do at the moment?

  1. Speaking of which, as another comment, it seems like this section should just be entirely replaced/deleted now and be part of the instructions on CLI use of SasView in general (using for example jupyter notebooks?

Remember that sans_to_sesans.rst & sesans_fitting.rst are part of Sasmodels which is agnostic in respect of Sasview. But also see SasView/sasview#1931.

My point exactly. We do not tell people to install the full developer environment in order to use sasmodels in scripting mode do we? though perhaps a better question for @wpotrzebowski 😃 My real point was this should just be part of the CLI instructions eventually.... or do we have them in rst? I know we have a number of presentations in the documents repo and some demonstration scripts for sasmodels (as well as one for PR in sasview) but maybe we don't have rst describing the CLI interface/usage? If not maybe that needs to be a ticket?

That said. It was meant as a discussion point. I do not think that level of change is one to do for the next release - that would be crazy talk 😄

Now to check what all has been done and move this to done ...

@smk78
Copy link
Contributor Author

smk78 commented Oct 28, 2021

@butlerpd commented:

We do not tell people to install the full developer environment in order to use sasmodels in scripting mode do we? though perhaps a better question for @wpotrzebowski 😃 My real point was this should just be part of the CLI instructions eventually.... or do we have them in rst? I know we have a number of presentations in the documents repo and some demonstration scripts for sasmodels (as well as one for PR in sasview) but maybe we don't have rst describing the CLI interface/usage? If not maybe that needs to be a ticket?

There is scripting.rst which becomes https://www.sasview.org/docs/user/qtgui/Perspectives/Fitting/scripting.html.

@butlerpd
Copy link
Member

sasmodels ready for testing on Win

@butlerpd
Copy link
Member

Right I give ... I will cheat and make a proper build to review. my installation is also breaking ☹️

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 now see what you mean @smk78 about the "script" for SESANS being different. I'd forgotten this legacy bit. Am thinking that sesans_fitting.rst should eventually be removed along with the GUI code that was dumped into sasmodels quick and dirty like to provide a GUI before we got the SasView GUI working? But for now it exists so the documentation should be fixed appropriately.

This should be ready to merge except for the two minor issues noted.

Comment on lines 85 to 86
time-of-flight measurements, which is the reason that not the polarisation is
plotted, but the :math:`\frac{log(P/P_0)}{\lambda^2}` . The sample is a
Copy link
Member

Choose a reason for hiding this comment

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

that sounds funny? Maybe "that the polarisation ins not plotted," ?

@butlerpd
Copy link
Member

Right ... it did not remember my major comment. Lets try that again shall we (clearly I'm not good with the tools here ☹️

using these instructions.

It is possible to fit SESANS measurements from the command line in Python.
(http://trac.sasview.org/wiki/DevNotes/DevGuide) is a prerequisite for
Copy link
Member

Choose a reason for hiding this comment

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

Actually we should point to the github wiki as track is in principle deprecated and the instructions there quite stale anyway. There is even the question of whether to bother attempting to migrate it to danse2 prior to wiping danse.chem.utk.edu. correct link would be:
https://github.com/SasView/sasview/wiki/DevNotes_DevEnviroment

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 changes requested do not warrant another review. I should have approved. Can I change it by submitting an approval without resolving?

@smk78
Copy link
Contributor Author

smk78 commented Oct 29, 2021

Replying to @butlerpd

"that the polarisation ins not plotted,"

I suspect that arose from the original Dutch! :-) Now changed to "...that the polarisation is not plotted..."

Have also changed the trac link to point to the Github wiki

@wpotrzebowski wpotrzebowski merged commit c746947 into master Oct 29, 2021
@wpotrzebowski wpotrzebowski deleted the DocUpdateSESANS branch October 29, 2021 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants