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

Largest, not first maximum used in corfunc #2457

Conversation

lucas-wilkins
Copy link
Contributor

As the title says (based on #2450)

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)

@lucas-wilkins lucas-wilkins added the Corfunc Perspective Concerns Correlation Function Perspective label Mar 13, 2023
@lucas-wilkins lucas-wilkins requested a review from smk78 March 13, 2023 09:30
@lucas-wilkins
Copy link
Contributor Author

@smk78 Change made, but I didn't have the wobbles you did for your input file.

@lucas-wilkins lucas-wilkins marked this pull request as ready for review March 13, 2023 13:49
@lucas-wilkins lucas-wilkins added Discuss At The Call Issues to be discussed at the fortnightly call Bug Fix and removed Discuss At The Call Issues to be discussed at the fortnightly call labels Mar 14, 2023
@butlerpd
Copy link
Member

Needs looking at.

@butlerpd butlerpd removed the Discuss At The Call Issues to be discussed at the fortnightly call label Mar 14, 2023
@smk78
Copy link
Contributor

smk78 commented Mar 14, 2023

Is this change in #2463 or do I have to review this and that?

@lucas-wilkins lucas-wilkins mentioned this pull request Mar 15, 2023
7 tasks
Copy link
Contributor

@smk78 smk78 left a comment

Choose a reason for hiding this comment

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

This change is in #2463 anyway.

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 minor suggestions with the code. I haven't tested functionality, but @smk78 seems to be doing a good job of that.

src/sas/sascalc/corfunc/corfunc_calculator.py Show resolved Hide resolved
def supplementary(self, supplementary_data: SupplementaryParameters):
self._supplementary = supplementary_data

self.draw_data() # Is this needed?
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it needed? I only ask because you are asking yourself here.


The 1d correlation function in self.data, the 3d correlation function
in self.data3, and the interface distribution function in self.data_idf
are all draw in on the plot in linear cooredinates."""
Copy link
Contributor

Choose a reason for hiding this comment

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

typo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but not mine

@lucas-wilkins
Copy link
Contributor Author

Thanks for reviewing @krzywon but I should have closed before, as its all in #2463

@krzywon
Copy link
Contributor

krzywon commented Mar 16, 2023

Thanks for reviewing @krzywon but I should have closed before, as its all in #2463

Thanks for the heads up. Reviewing #2463 now.

@lucas-wilkins lucas-wilkins deleted the 2453-corfunc-parameter-extraction-is-not-correctly-identifying-the-long-period branch March 17, 2023 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Corfunc Perspective Concerns Correlation Function Perspective
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Corfunc parameter extraction is not correctly identifying the long period
4 participants