Change objective and reference naming#74
Conversation
Codecov Report
@@ Coverage Diff @@
## main #74 +/- ##
==========================================
- Coverage 74.15% 73.96% -0.20%
==========================================
Files 35 35
Lines 1389 1398 +9
==========================================
+ Hits 1030 1034 +4
- Misses 359 364 +5
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| group.add_option( | ||
| '--usefilenames', | ||
| action="store_true", | ||
| dest="usefilenames", | ||
| help="Use the file names as labels on plot." | ||
| ) | ||
| group.add_option( | ||
| '--mlabel', | ||
| metavar="MLABEL", | ||
| dest="mlabel", | ||
| help="Set label for morphed data to MLABEL on plot. Ignored if using file names as labels.", | ||
| ) | ||
| group.add_option( | ||
| '--tlabel', | ||
| metavar="TLABEL", | ||
| dest="tlabel", | ||
| help="Set label for target data to TLABEL on plot. Ignored if using file names as labels.", | ||
| ) |
There was a problem hiding this comment.
New option to set label names on plot (for fixing #46).
There was a problem hiding this comment.
looks good, see comment. Please put different issues on different PRs
sbillinge
left a comment
sbillinge
left a comment
There was a problem hiding this comment.
Changes look great, thanks!
Please see quick inline comments and I can merge.
Please put less on each PR to make it easier to review and merge. One thing per branch/pr ideally. If something is a quick cleaning thing you can add it to another one, but this should have been at least two PRs if not three.
Thanks so much
| @@ -24,7 +24,7 @@ | |||
| class MorphXtalPDFtoRDF(Morph): | |||
There was a problem hiding this comment.
this is not a "morph" this is simply a transformation. Please can we change this language? I want to merge this so please make an issue to change the language (and name) on this.
Having said that, leave all the changes you have done and I will merge it, but let's think a bit about the class name and the docstring/documentation. I should probably understand exactly what is being done here before we finalize this, but stick it on an issue (and add it to the release milestone) so we loop back to it
There was a problem hiding this comment.
Yep, I'll open a new issue for this (and below)
There was a problem hiding this comment.
I am having trouble adding the issue to the milestone
| @@ -24,7 +24,7 @@ | |||
| class MorphXtalRDFtoPDF(Morph): | |||
| group.add_option( | ||
| '--usefilenames', | ||
| action="store_true", | ||
| dest="usefilenames", | ||
| help="Use the file names as labels on plot." | ||
| ) | ||
| group.add_option( | ||
| '--mlabel', | ||
| metavar="MLABEL", | ||
| dest="mlabel", | ||
| help="Set label for morphed data to MLABEL on plot. Ignored if using file names as labels.", | ||
| ) | ||
| group.add_option( | ||
| '--tlabel', | ||
| metavar="TLABEL", | ||
| dest="tlabel", | ||
| help="Set label for target data to TLABEL on plot. Ignored if using file names as labels.", | ||
| ) |
There was a problem hiding this comment.
looks good, see comment. Please put different issues on different PRs
Looked over inline comments; I will put less in each PR in the future, sorry for bundling them all together this time |
2 participants
Fixes #37. Fixes #46. Fixes #55.