feat: getmud UC1 (get_diameter) #184
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #184 +/- ##
==========================================
+ Coverage 99.31% 99.32% +0.01%
==========================================
Files 5 6 +1
Lines 292 298 +6
==========================================
+ Hits 290 296 +6
Misses 2 2
🚀 New features to boost your workflow:
|
| xray_energy=args.energy, | ||
| sample_mass_density=args.mass_density, | ||
| ) | ||
| print(f"Capillary Diameter: {diameter:.2f} mm") |
There was a problem hiding this comment.
The current logic is that if the user enters get-diameter, it will load the compute_diameter function from getmud module. I'm not sure if printing the result is the best approach here. The CLI will look something like labpdfproc get-diameter 2 ZrO2 17.45 1.6.
| "get-diameter", help="Compute capillary diameter." | ||
| ) | ||
| get_d_parser.add_argument( | ||
| "mud", type=float, help="The target muD of the sample." |
There was a problem hiding this comment.
I will write a better help message with an example here
I don't think this closes #166 which has multiple UCs in it. IF those UCs have been broken out as we intended, then we can manually close #166, and in this PR you want to close whicher of those granular issues this PR addresses. |
|
Is this an old PR somehow left over from before our conversation? I am looking forward to having a series of small PRs that do each of the sub-parts of the UCs we were discussion, and we can just close this PR? I left a few comments, but I htink they may not be relevant if we are doing the granular PRs |
Yes, the old PR is in #175. As discussed, we need to separate PRs for UC1, UC3+4, CLI, and docs. This PR includes UC1 and its CLI app. I kept them in the same PR because the CLI directly calls the function, and having them together makes it easier to check how it works. Let me know if you think this makes sense! |
|
I think it is better if we split it up, one function per pr. Pls see my comment on this one, but the cli will be last because it won't work till all the functions are written and it can call them one after the other |
Okay. I didn't see any specific comments on this PR? |
src/diffpy/labpdfproc/getmud.py
Outdated
| from diffpy.utils.tools import compute_mu_using_xraydb | ||
|
|
||
|
|
||
| def compute_diameter( |
There was a problem hiding this comment.
I think this function should just be
def get_diameter(mud, mu)
return mud/mu
with docstrings etc. of course.
There was a problem hiding this comment.
I think previously we discussed that user enters chemical info related to mu, but not mu itself (See #175 (review))? Maybe a better approach here is to write return mud/compute_mu(composition, energy, density)?
There was a problem hiding this comment.
A function that computes mu Is much more reusable. It should probably be in didn't. Utils though
There was a problem hiding this comment.
@sbillinge Sounds good, I edited the function and it's ready for another review. I removed the CLI here and created a new issue for it (#186).
yucongalicechen
changed the title
| diameter : float | ||
| Computed capillary diameter in mm. | ||
| """ | ||
| return mud / compute_mu_using_xraydb( |
There was a problem hiding this comment.
Better format - use compute_mu function from diffpy.utils.
|
I don't think we need this function. If we have a We just have to decide what we want the user experience to be for that. A standalone app that just does that and has its own name, or having it as an optional part of labpdfrpoc. I can think about what I htink is better, but I don't have a clear idea. Maybe you can think about it too. |
Address UC1 in #166
@sbillinge ready for review