feat: initial commit on getmud module #175
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #175 +/- ##
==========================================
+ Coverage 99.31% 99.33% +0.02%
==========================================
Files 5 6 +1
Lines 292 302 +10
==========================================
+ Hits 290 300 +10
Misses 2 2
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
please see comments. I am a little confused what is going on here. Do we have UCs for these functionalities? What is the user wanting to do? Once we know that we can design what the functions should do and capture that in tests. Only then do we write any functions.
tests/test_getmud.py
Outdated
| "mud": 2.0, | ||
| "diameter": 1.5, | ||
| "sample_composition": "ZrO2", | ||
| "energy": 20, |
There was a problem hiding this comment.
it is probably a better test if we use reasonable energies. These would be Cu Kalpha, Mo Kalpha, Ag Kalpha. They don't have to be exact but ballpark,
| mu = compute_mu_using_xraydb( | ||
| sample_composition, energy, sample_mass_density, packing_fraction | ||
| ) | ||
| return mud / mu |
| diameter : float | ||
| The given diameter of the sample capillary in mm. | ||
| """ | ||
| mu = compute_mu_using_xraydb( |
There was a problem hiding this comment.
comment out and replace with pass. Let's just work on tests to begin with, until we learn what we want the function to do.
| mud : float | ||
| The given product of attenuation coefficient mu | ||
| in mm^{-1} and capillary diameter in mm. | ||
| sample_composition : str |
There was a problem hiding this comment.
These are not needed presumably. If I know mud and I know mu then nothing else matters.
|
@sbillinge ready for review - I edited the docstrings and tests based on our UC1-2. |
where can I find the UCs? Please can you put a link here? |
It's here: #166. |
There was a problem hiding this comment.
OK, this is great now we have the UCs. I think we are doing UC1 and UC3 and UC4.
UC1 is composition+energy+rho -> diameter
UC3 is comp + E -> mu (fully dense and 50% packing fraction)
I think that we probably want:
F1: comp + E + density -> mu
F2: comp + E -> estimated mu after looking up a CIF
Then we can satisfy
UC1 as muD/F1(comp,E,density)
UC4 as muD/F2(comp,E)
F2 satisfies UC3
Please could you close this and make one PR for each of those functions?
Do we also need a command line app that the user runs to get this info? Let's put that on a separate PR, with the docs too.
It is already 3:20 so I guess we won't get this merged by 5. Shall we push off that meeting until tomorrow?
| Parameters | ||
| ---------- | ||
| mud : float | ||
| The given product of attenuation coefficient mu |
There was a problem hiding this comment.
"The given muD of the sample"
The rest you wrote just makes it more confusing I thank.
| diameter : float | ||
| The given diameter of the sample capillary in mm. | ||
| sample_composition : str | ||
| The chemical formula of the material. |
There was a problem hiding this comment.
I would give a few example to illustreate what form it must be in to be valid. I think we will use that help text in multiple places, so maybe define it in a variable and reuse? We may have already written it somewere in a docstring or the docs, so just copy paste from there if possible....
| The given diameter of the sample capillary in mm. | ||
| sample_composition : str | ||
| The chemical formula of the material. | ||
| energy : float |
There was a problem hiding this comment.
maybe call this xray_energy?
|
|
||
| Returns | ||
| ------- | ||
| estimated_density : float |
There was a problem hiding this comment.
I don't think the density is "estimated", it is calculated, given the other parameters
Gotcha yeah I will make one PR for each function! Will find a spot for meeting tomorrow :) |
|
@sbillinge Just to clarify before I make any new PRs, I will do the following:
|
|
That looks about right, but I think there needs to be a CLI app for UC1 also, no? We should maybe discuss what the CLI should look like. Are they separate apps that do those tasks, or do we want one entry point but give different behavior if different command line switches are used? For example, for UC1 should the user type |
Gotcha gotcha I think I'm on the same page now. I've created a PR for this (#184), so I'm closing this one. |
closes #166
Initial commit. Probably needs a bit more discussion on what's the most useful here.