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 in Fourier filter capability #320

Closed
wants to merge 1 commit into from
Closed

Conversation

Kvieta1990
Copy link
Collaborator

@Kvieta1990 Kvieta1990 commented Dec 30, 2020

The Fourier filter capability is added to ADDIE interface through underlying pystog module. Conda recipe needs to be updated accordingly to add in the dependency upon pystog.

I will be waiting for @marshallmcdonnell to sort out the pystog conda package issue.

Also, current implementation only applies Fourier filter in real-space. Need to discuss with Matt whether we want the Fourier filter to be reflected in Q-space as well. Will put in corresponding implementation if needed.

Related to issue #91

@Kvieta1990 Kvieta1990 added GUI: Calcualte G(r) Tab The Calculate G(r) Tab Status: PyStoG Work Needed WIP depends on PyStoG work being complete first Type: Enhancement Added feature or enhancement of current features labels Dec 30, 2020
@marshallmcdonnell
Copy link
Member

Okay, looking at this further (since I am finishing up the PyStoG stuff), I am seeing stuff I didn't realize before:

  1. The fourier filter is being applied to all PDF functions. I think it should be optional. The User should be able to control this via the GUI (see Issue Generate S(Q) button to do reverse transform from reciprocal -> real space #91 for the original idea)

  2. The filter is being carried out to the minimum R (Rmin box) that is specified in the GUI. Yet, this is meant to control the integration limit. Maybe it is okay to "piggyback" off this value but they serve two different purposes. I think this should be discussed further. Why? You want to possibly not apply a fourier filter but change the limit of integration.

  3. Related to the following comment:

Also, current implementation only applies Fourier filter in real-space. Need to discuss with Matt whether we want the Fourier filter to be reflected in Q-space as well. Will put in corresponding implementation if needed.

I think we should give the Q-space function as well (not over-write the original but provide the new one) since you want to provide reciprocal space function that gave you the real space function via the fourier transform. Will need this for analysis in programs that can fit the pair (like RMCProfile). Right now, the User will be left with a reciprocal space function that does not have the filter applied and the real space function that does have this applied, thus not an equal pair.

@marshallmcdonnell
Copy link
Member

And now that I review Issue #91, honestly, I think the first step should be just to add a back transform from real space to reciprocal space [i.e. G(r) -> S(Q)].

From that, you can actually perform the fourier filter yourself in the GUI. Yet, I do want to include your feature addition of a "quick" fourier filter button / setup to the GUI

Copy link
Member

@marshallmcdonnell marshallmcdonnell left a comment

Choose a reason for hiding this comment

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

Overall, I think we need to discuss more (see above comments).

Big points are:
The fourier filter needs to be optional and controlled via the GUI.
We probably should solve issue #91 first as a "first step".
The current implementation would give the User the incorrect real space, reciprocal space pair of functions [an S(Q) that the G(r) did not come from]

@marshallmcdonnell
Copy link
Member

Update:
Putting issue #91 in backlog.
From offline discussion with @Kvieta1990 and Matt Tucker, that feature is not important.

Changes that still need to happen:

  • The fourier filter needs to be optional and controlled via the GUI.
  • Include the plot of the fourier filtered S(Q) and G(r) if we do apply the fourier filter

@Kvieta1990
Copy link
Collaborator Author

Will close this PR and create a new one, putting in stuff based on our latest discussion as summarized by @marshallmcdonnell below,

Update:
Putting issue #91 in backlog.
From offline discussion with @Kvieta1990 and Matt Tucker, that feature is not important.

Changes that still need to happen:

  • The fourier filter needs to be optional and controlled via the GUI.
  • Include the plot of the fourier filtered S(Q) and G(r) if we do apply the fourier filter

@Kvieta1990 Kvieta1990 closed this Jan 8, 2021
@marshallmcdonnell marshallmcdonnell deleted the add_fourier_filter branch January 15, 2021 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GUI: Calcualte G(r) Tab The Calculate G(r) Tab Status: PyStoG Work Needed WIP depends on PyStoG work being complete first Type: Enhancement Added feature or enhancement of current features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants