-
Notifications
You must be signed in to change notification settings - Fork 11
feat: options for estimating muD theoretically #173
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please see comments
2. Estimate muD from a z-scan file (`-z` or `--z-scan-file`). | ||
2. Estimate from a z-scan file (`-z` or `--z-scan-file`). | ||
3. Estimate theoretically based on sample mass density | ||
(`-td` or `--theoretical-from-density`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check but this is breaking the usual pattern that it should be -l
(one letter) or --letters
(multiple letters need multiple dashes.
"and sample mass density (in g/cm^3), in that exact order, " | ||
"separated by commas (e.g., ZrO2,20,1.5). " | ||
"If you add whitespaces, " | ||
"enclose it in quotes (e.g., 'ZrO2, 20, 1.5'). " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
allow whitespace - edits in help message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the parser do if there is whitespace but no quotes. Did you test that?
@sbillinge ready for another review! I addressed the comment about the whitespace and edited help messages accordingly. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #173 +/- ##
=======================================
Coverage 99.31% 99.31%
=======================================
Files 5 5
Lines 292 292
=======================================
Hits 290 290
Misses 2 2
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please see comments
@@ -21,12 +21,13 @@ def _define_arguments(): | |||
"The filename(s) or folder(s) of the datafile(s) to load. " | |||
"Required.\n" | |||
"Supply a space-separated list of files or directories. " | |||
"If a filename contains whitespace, enclose it in quotes. " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe mention that, as a general matter, avoid filenames with whietspaces in, but if it does, then enclose in quotes
"and sample mass density (in g/cm^3), in that exact order, " | ||
"separated by commas (e.g., ZrO2,20,1.5). " | ||
"If you add whitespaces, " | ||
"enclose it in quotes (e.g., 'ZrO2, 20, 1.5'). " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the parser do if there is whitespace but no quotes. Did you test that?
"in that exact order " | ||
"and separated by commas with no whitespaces " | ||
"(e.g., 'ZrO2,2,0.5')." | ||
"Specify the chemical formula, incident x-ray energy (in keV), " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since this seems to be used 2x, define it in a variable and call the variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbillinge ready for another review. I edited the help messages according to the comments above. When there's whitespace but no quotes the CLI will raise an error saying that it doesn't recognize the command
…ergy
"separated by commas (e.g., ZrO2,17.45,0.5). " | ||
"If you add whitespaces, " | ||
"enclose it in quotes (e.g., 'ZrO2, 17.45, 0.5'). " | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
common part for help messages
@@ -21,12 +28,14 @@ def _define_arguments(): | |||
"The filename(s) or folder(s) of the datafile(s) to load. " | |||
"Required.\n" | |||
"Supply a space-separated list of files or directories. " | |||
"Avoid spaces in filenames when possible; " | |||
"if present, enclose the name in quotes. " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
edit input help message
closes #143
@sbillinge ready for review
Here's a gui snapshot:
