add fast_cve to labpdfprocapp.py and some other fixes #87

yucongalicechen · 2024-07-25T16:41:37Z

closes Implement quick calculation #85 (changed default calculation to fast cve while giving an option of -b to enable brute-force calculation. system exit if mu*D is out of range for 0.5-6.)
fixed a few other issues: it seems that we accidentally deleted the labpdfproc command last time (so we cannot run the last released version using labpdfproc). also the default value for args.output_correction was wrong.

…-range error

sbillinge

looks good. Please see comment.

src/diffpy/labpdfproc/labpdfprocapp.py

sbillinge

I am not sure I like this refactoring tbh as it makes the brute_force and fast_ calculations assymmetric.

I think a better design is to have different ways of computing the cve, all of which have the same signature and return the same data-types (and units) and when they are applied the "factory method" (but it can just be an if-statement) just figures out which to call.

You can decide whether we do both on the data grid (so pass in hte data-grid) or both on a 0.1 grid and do the interpolation outside, but I think this would be clearer and easier to maintain and extend (for example, if we come up with a new way of computing that we want to add.

Also, why does fast_cve have its own module? Either put it in functions.py or make a module of cve_calculators.py, but probably the former unless it is getting too long and messy.

yucongalicechen · 2024-07-29T17:10:49Z

I am not sure I like this refactoring tbh as it makes the brute_force and fast_ calculations assymmetric.

I think a better design is to have different ways of computing the cve, all of which have the same signature and return the same data-types (and units) and when they are applied the "factory method" (but it can just be an if-statement) just figures out which to call.

You can decide whether we do both on the data grid (so pass in hte data-grid) or both on a 0.1 grid and do the interpolation outside, but I think this would be clearer and easier to maintain and extend (for example, if we come up with a new way of computing that we want to add.

Also, why does fast_cve have its own module? Either put it in functions.py or make a module of cve_calculators.py, but probably the former unless it is getting too long and messy.

Okay, yes I agree it is confusing to define different tth grids. I put the fast_cve module into functions with an if-else statement on which method to use. Should be more readable now!

sbillinge

please see my comments. This seems to be heading a little bit in the wrong direction. If you want to chat about the design we can, or if you have it figured out from my comments, that is fine too.

It is easy to fix.

src/diffpy/labpdfproc/functions.py

…rs _cve_brute_force or _cve_interp_polynomial

sbillinge

please see comments

src/diffpy/labpdfproc/functions.py

sbillinge · 2024-08-23T11:15:29Z

src/diffpy/labpdfproc/functions.py

+    else:
+        return _cve_interp_polynomial(diffraction_data, mud, wavelength)
+
+
 def compute_cve(diffraction_data, mud, wavelength, brute_force=False):


This function naming is a bit confusing. I think the cve is computed in the function you call _cve_method, so that should be called _compute_cve. This function seems to do something else, like take a computed cve and put it into a diffraction object.

Let's maybe have a quick conversation about this.

Does _interpolate_cve sound good?

I added the method argument to args and cleaned up the four relevant functions in functions.py (_cve_brute_force, _cve_polynomial_interpolation, _compute_cve, and interpolate_cve).

I added the method argument to args and cleaned up the four relevant functions in functions.py (_cve_brute_force, _cve_polynomial_interpolation, _compute_cve, and interpolate_cve).

I guess my question is whether we need this function. I feel like I want a function where I give it a grid (in the form of a DO) and it gives me a cve, and a function that I give a diffraction pattern and a cve on hte same grid as each other and it gives me a corrected pattern, regardless of what backend is used. So the interpolation step is not needed for the brute-force case, and it probably should be done as part of the cve calculation in the fast cve calculator? I am interested in the cleanest interface for the user.

I think this function is necessary. If we use the original grid for brute-force calculations, it could take forever to run (so I think the interpolation would be necessary), and the interpolation is accurate since the numbers are small.
If we're interested, I can explore whether we can write cve as a continuous function of angle so that we can remove the interpolation?

that's ok, I don't mind having an interpolation. I just don't like the current structure because it gives a messy api and difficult to follow code. btw, we might be able to interpolate onto different grids in the DO itself. I wanted to have that capability but not sure if I implemented it.

yucongalicechen · 2024-08-28T18:49:17Z

I updated the functions so that they now pass around DOs. There are some duplicated codes but I think it might good to have some comments on the current structure first.

sbillinge

please see inline comments. Sorry, I started a review before but didn't finish it.

src/diffpy/labpdfproc/functions.py

sbillinge · 2024-09-04T08:59:35Z

src/diffpy/labpdfproc/functions.py

+    return methods[method]
+
+
+def compute_cve(diffraction_data, mud, method="polynomial_interpolation"):


Good practice here is to make the default as method=None then in the code do

If method is None: method = "polynomial_interpolation"

src/diffpy/labpdfproc/functions.py

sbillinge · 2024-09-04T09:04:42Z

src/diffpy/labpdfproc/labpdfprocapp.py

+        "-m",
+        "--method",
+        help=f"The method for computing absorption correction. Allowed methods: {*CVE_METHODS, }. "
+        f"Default method is polynomial interpolation if not specified. ",


is there an indent problem here?

src/diffpy/labpdfproc/labpdfprocapp.py

sbillinge · 2024-09-04T09:07:31Z

src/diffpy/labpdfproc/tools.py

    it is determined as follows:
    If user provides an output directory, use it.
    Otherwise, we set it to the current directory if nothing is provided.
    We then create the directory if it does not exist.

+    Returns
+    -------
+    pathlib.PosixPath that contains the full path of the output directory


why are we returning posixPath and not just Path? We want it to be platform independent, no?

src/diffpy/labpdfproc/tools.py

sbillinge · 2024-09-04T09:11:53Z

@yucongalicechen this PR is getting a bit long and complicated. We can split some of the tasks into other PRs if we like,like outputting chi and not xy files. Make an issue to do that and note it in the conversation, then we can resolve the conversation and try and resolve all the conversations either by code fixes or issues, then we can merge this. It is in the right direction now.

yucongalicechen · 2024-09-04T17:09:13Z

Sounds great, I'll break these into different issues and PRs and let you know!

yucongalicechen · 2024-09-05T19:18:30Z

@sbillinge I think I addressed all comments here in PR #89, #92, and #93, except that there's one comment about the posixPath I haven't got to.
We have output_dir=Path(...).resolve() so it is the absolute Path, so in the docstring I'll correct it as a Path object.
I want to make sure that we're happy with having output directory as a Path object instead of a string object?

sbillinge · 2024-09-05T20:33:27Z

@sbillinge I think I addressed all comments here in PR #89, #92, and #93, except that there's one comment about the posixPath I haven't got to. We have output_dir=Path(...).resolve() so it is the absolute Path, so in the docstring I'll correct it as a Path object. I want to make sure that we're happy with having output directory as a Path object instead of a string object?

Yes if it works.

yucongalicechen · 2024-09-05T21:49:19Z

Yes I think Path works better than string. I edited the docstring in #96.

yucongalicechen · 2024-09-06T13:21:52Z

closed by PR #89, #92, #93, and #96.

yucongalicechen added 4 commits July 25, 2024 12:07

add back labpdfproc command

329a895

change default to fast cve, add brute-force option, handle muD out-of…

6954a29

…-range error

minor correction on output correction command

97bcc00

add brute force argument to metadata test

2487ae6

sbillinge reviewed Jul 26, 2024

View reviewed changes

src/diffpy/labpdfproc/labpdfprocapp.py Outdated Show resolved Hide resolved

yucongalicechen added 3 commits July 26, 2024 13:13

remove duplicated functions in fast cve

7d0d574

test function for fast_compute_cve

b7e037e

fix misplaced docstring

a450dda

sbillinge reviewed Jul 27, 2024

View reviewed changes

integrate fast cve module in functions

fc4d876

sbillinge reviewed Jul 30, 2024

View reviewed changes

src/diffpy/labpdfproc/functions.py Outdated Show resolved Hide resolved

src/diffpy/labpdfproc/functions.py Outdated Show resolved Hide resolved

src/diffpy/labpdfproc/functions.py Outdated Show resolved Hide resolved

add private function _cve_method in compute_cve to select cve compute…

956408d

…rs _cve_brute_force or _cve_interp_polynomial

sbillinge reviewed Aug 23, 2024

View reviewed changes

yucongalicechen added 6 commits August 26, 2024 15:38

add method to argparser and correct functions and docstrings

4bbe4a2

change cwd path from os to pathlib

b91b6ec

add method argument to metadata test

e7d3d05

change compute cve function to call cve methods and interpolate tth

bb28204

updated compute_cve to use DOs consistently

3c6b128

change output file format from .chi to .xy

377607a

sbillinge reviewed Sep 4, 2024

View reviewed changes

This was referenced Sep 4, 2024

incorporate different cve computation methods to compute_cve #91

Closed

compute_cve #92

Merged

minor fixes #93

Merged

yucongalicechen closed this Sep 6, 2024

yucongalicechen deleted the fast_cve branch September 6, 2024 13:21

yucongalicechen mentioned this pull request Oct 28, 2024

compute_cve replace the interpolation step #94

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add fast_cve to labpdfprocapp.py and some other fixes #87

add fast_cve to labpdfprocapp.py and some other fixes #87

yucongalicechen commented Jul 25, 2024

sbillinge left a comment

sbillinge left a comment

yucongalicechen commented Jul 29, 2024

sbillinge left a comment

sbillinge left a comment

sbillinge Aug 23, 2024

yucongalicechen Aug 26, 2024

yucongalicechen Aug 26, 2024

sbillinge Aug 26, 2024

yucongalicechen Aug 26, 2024

sbillinge Aug 27, 2024

yucongalicechen commented Aug 28, 2024

sbillinge left a comment

sbillinge Sep 4, 2024

sbillinge Sep 4, 2024

sbillinge Sep 4, 2024

sbillinge commented Sep 4, 2024

yucongalicechen commented Sep 4, 2024

yucongalicechen commented Sep 5, 2024

sbillinge commented Sep 5, 2024

yucongalicechen commented Sep 5, 2024

yucongalicechen commented Sep 6, 2024

		return methods[method]


		def compute_cve(diffraction_data, mud, method="polynomial_interpolation"):

add fast_cve to labpdfprocapp.py and some other fixes #87

add fast_cve to labpdfprocapp.py and some other fixes #87

Conversation

yucongalicechen commented Jul 25, 2024

sbillinge left a comment

Choose a reason for hiding this comment

sbillinge left a comment

Choose a reason for hiding this comment

yucongalicechen commented Jul 29, 2024

sbillinge left a comment

Choose a reason for hiding this comment

sbillinge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yucongalicechen commented Aug 28, 2024

sbillinge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbillinge commented Sep 4, 2024

yucongalicechen commented Sep 4, 2024

yucongalicechen commented Sep 5, 2024

sbillinge commented Sep 5, 2024

yucongalicechen commented Sep 5, 2024

yucongalicechen commented Sep 6, 2024