compute_cve #92
compute_cve #92
Conversation
yucongalicechen
commented
Sep 4, 2024
yucongalicechen
commented
- closes incorporate different cve computation methods to compute_cve #91
- continue from add fast_cve to labpdfprocapp.py and some other fixes #87. I thought it'd be cleaner to start from a new branch
- addressed comments: use a more understandable name than abdo, add cve_methods to docstring.
- I haven't changed the default method to None. When we load the args into headers we will write the method, so if we want to have default method to None, we need a function to change args.method=None to args.method="polynomial_interpolation". Is it easier if we just have the default value as "polynomial_interpolation"?
There was a problem hiding this comment.
nicely done! There is one inline comment.
Also, codecov is failing. We need some tests for when things go wrong. This would be in a separate test called test_compute_cve_bad. These tests are not strictly necessary but definitely make the code better so let's do them while we are at it.
Let's also make an issue to replace the interpolation step with one using diffraction objects if that ever gets implemented. I think there should be an issue in diffpy.utils for that so link to that issue.
| "-m", | ||
| "--method", | ||
| help=f"The method for computing absorption correction. Allowed methods: {*CVE_METHODS, }. " | ||
| f"Default method is polynomial interpolation if not specified. ", |
There was a problem hiding this comment.
I am worried about the indentation of this line. I am not sure if it will break things or not, but but it would be better indented to line up with the line above in any case.
There was a problem hiding this comment.
black seems to automatically adjust the indentation for all help messages and aligns them at help. I can try to add a paranthesis around the messages to make sure they line up with the lines above, if that's helpful?
There was a problem hiding this comment.
I've fixed the indentation issue in the latest commit. There are some conflicts with labpdfprocapp.py, but my local branch shows that the code is up-to-date when I try to merge from main to resolve conflicts. I think we can accept the edits for labpdfprocapp.py in this PR.
There was a problem hiding this comment.
No, we have to fix it. Did you commit to main by mistake? That could cause this behavior
There was a problem hiding this comment.
Okay, I managed to resolve conflicts. I think it just needs me to merge from main again.
|
@yucongalicechen one more thing, we need a news for this. |
|
Nice, thanks! |
