Skip to content
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

Revert "Add processing modules after PEP updates & test_rixs.py" #8

Merged
merged 1 commit into from
Jul 14, 2018

Conversation

awalter-bnl
Copy link
Contributor

Reverts #4

I want to revert these changes as there is a lot still to work through before they are ready to merge, and I haven't been given enough time to check through the code myself yet.

@codecov-io
Copy link

Codecov Report

Merging #8 into master will decrease coverage by 5.1%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #8      +/-   ##
==========================================
- Coverage   48.37%   43.26%   -5.11%     
==========================================
  Files           5        3       -2     
  Lines         430      282     -148     
==========================================
- Hits          208      122      -86     
+ Misses        222      160      -62

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 75c2dcf...f394512. Read the comment docs.

@awalter-bnl
Copy link
Contributor Author

I have added some comments to PR #4 that I would like to see changed

@mpmdean
Copy link
Contributor

mpmdean commented Jul 13, 2018

I think this addresses the points as best I can in the new commit.

I am keen to be able to share the code and have Andi, Valentina, etc. start running it on real data. We have an experiment in only one week and if we don't have something shared between us, we will each be separately trying to hack together different bits of code.

@awalter-bnl
Copy link
Contributor Author

I will take a look at this early next week when I have the chance, as this is the first full PR to scikit-beam/rixs I would also like feed back from @danielballan and/or @tacaswell . I have requested a review from them.

As mentioned in the meeting earlier this week, having this stuff ready for this cycle is unrealistic. Having it ready for later this week is really not possible given my schedule. If you would like others to use it before it is merged they can fork your branch (or the revert-4-master branch if you didn't create your own fork) prior to this being merged into the master branch.

@awalter-bnl awalter-bnl requested review from tacaswell and danielballan and removed request for tacaswell and danielballan July 14, 2018 10:05
@awalter-bnl
Copy link
Contributor Author

I just realized that there are no commits to this PR and the only other open PR is my initial one. @mpmdean can you please merge this PR and open a new PR with the changed files in it as this will make it easier for others to follow.

@mpmdean mpmdean merged commit 110a036 into master Jul 14, 2018
@danielballan
Copy link
Member

I would like to add that looking at the overall quality of this code makes me very happy: well-formatted docstrings, at least some tests, generally well-factored function signatures. This is off to a great start.

I think @awalter-bnl is right about process. It's good to go through revision before merging.

If you would like others to use it before it is merged they can fork your branch (or the revert-4-master branch if you didn't create your own fork) prior to this being merged into the master branch.

If you need help with that let us know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants