Python Interfacing for PDFmorph (Clean Branch) #191
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #191 +/- ##
==========================================
+ Coverage 97.44% 97.58% +0.14%
==========================================
Files 20 21 +1
Lines 978 1078 +100
==========================================
+ Hits 953 1052 +99
- Misses 25 26 +1
🚀 New features to boost your workflow:
|
|
@Luiskitsu please could you look at this. We may have battling pianos here so i want to synchronize efforts. Also, this is a big PR that is coming from a bunch of work we were doing at another XFEL beamtime and it is hard to review such a big PR so more eyes on it would be good. In general, it would be good to arrange a quick face to face meeting to discuss everything and make sure we are pushing together in the same direction. |
|
Force push as previous branch not up to date with recent updates. |
|
I don't like merging force-pushes as they can break everything. Is there a reason you didn't merge to preserve the git history? |
|
There is some merged branch where the files morphpy and test_morphpy were touched (created then deleted), when they should not have been as this branch is supposed to introduce those files. Rebasing reset all the commits in those files prior to this branch, so either way commit history would've been lost. This branch had a lot more work, so I force pushed. No commits on this PRs history were touched by the force push. Note that the commit history of all other files (other than the two new ones) are preserved. |
|
@sbillinge Ready for review. Documentation added, and the new testdata file displays how the parameters of these two new morphs (squeeze and funcy) are displayed on command-line and when saved in the file information. Saving the morphing info can be toggled through the |
| # Shift | ||
| # Only enable hshift is squeeze is not enabled | ||
| if ( | ||
| opts.hshift is not None and squeeze_poly_deg < 0 | ||
| ) or opts.vshift is not None: | ||
| chain.append(morphs.MorphShift()) | ||
| if opts.hshift is not None and squeeze_poly_deg < 0: | ||
| hshift_in = opts.hshift | ||
| config["hshift"] = hshift_in | ||
| refpars.append("hshift") | ||
| if opts.vshift is not None: | ||
| vshift_in = opts.vshift | ||
| config["vshift"] = vshift_in | ||
| refpars.append("vshift") |
There was a problem hiding this comment.
Works better when shifts are done last.
|
Tutorial files (excluding the Full Parameter List): |
| assert pytest.approx(morph_info["funcy"]["s"]) == 2.5 | ||
|
|
||
|
|
||
| if __name__ == "__main__": |
There was a problem hiding this comment.
Let's just remove this in a future PR to get rid of this codecov problem
|
I am merging this as it is beautiful but a few comments. First, it is way too big of a PR making review very difficult. I would prefer it is it were on at least 3 PRs (maybe one per check-list point)! I don't want to look through it again, so I am just merging it, but to get rid the thing triggering codecov, we should just delete that "if name == main` block. It is not needed with pytest unless there is some special reason you are using it. |
|
Ill just remove that code. It's not being used by anything. |
That's true, the documentation and code should have definitely been split up. Integrating |
|
in general, one thing per PR is the best, except in rare circumstances |
Todo:
Closes #212