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.

Sign up for GitHub

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

Jump to bottom

Amor reflectometry #15

Merged
merged 61 commits into from
Apr 22, 2021
Merged

Amor reflectometry #15

merged 61 commits into from
Apr 22, 2021

Conversation

arm61
Copy link
Collaborator

@arm61 arm61 commented Apr 15, 2021

This is the first draft of the Amor/reflectometry reduction code. Somethings to note:

  • it is not feature complete
  • test coverage for the reflectometry modules is 100 % (not including the plotting stuff)
  • there is currently no testing for the amor package (this is a todo once the code is feature complete)

@arm61
Copy link
Collaborator Author

arm61 commented Apr 15, 2021

The CI is failing cause of line length (in comments and docstrings not in code) and one convenience import in a __init__.py. Any flexibility of flake8?

@arm61
Copy link
Collaborator Author

arm61 commented Apr 15, 2021

I am not familiar enough with yapf to know what it doesn't like now...seems to be all of the code 🤣

@arm61
Copy link
Collaborator Author

arm61 commented Apr 15, 2021

🎉

SimonHeybrock
SimonHeybrock reviewed Apr 16, 2021
View reviewed changes
Copy link
Member

@SimonHeybrock SimonHeybrock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part 1 (have not looked at all files yet):

src/ess/reflectometry/resolution.py Outdated Show resolved Hide resolved
src/ess/reflectometry/plot.py Outdated Show resolved Hide resolved
src/ess/reflectometry/__init__.py Outdated Show resolved Hide resolved
src/ess/amor/amor.py Outdated Show resolved Hide resolved
src/ess/amor/amor.py Outdated Show resolved Hide resolved
src/ess/reflectometry/corrections.py Outdated Show resolved Hide resolved
src/ess/reflectometry/corrections.py Outdated Show resolved Hide resolved
src/ess/reflectometry/corrections.py Outdated Show resolved Hide resolved
src/ess/reflectometry/corrections.py Outdated Show resolved Hide resolved
src/ess/reflectometry/corrections.py Outdated Show resolved Hide resolved
@arm61
Copy link
Collaborator Author

arm61 commented Apr 16, 2021

Part 1 (have not looked at all files yet):

Why I opened it now and not when feature complete!

@arm61
Copy link
Collaborator Author

arm61 commented Apr 21, 2021

That's test coverage back at 100 %.

@arm61 arm61 mentioned this pull request Apr 21, 2021
Merged
SimonHeybrock
SimonHeybrock approved these changes Apr 22, 2021
View reviewed changes
Copy link
Member

@SimonHeybrock SimonHeybrock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work for a "first draft" 👍

I assume there will be many more changes in the future, but I don't see anything wrong with merging this now.

@SimonHeybrock SimonHeybrock merged commit c5896aa into scipp:main Apr 22, 2021
@arm61
Copy link
Collaborator Author

arm61 commented Apr 22, 2021

🎉 🎉 🎉

@arm61 arm61 deleted the amor_reflectometry branch April 22, 2021 10:02
@SimonHeybrock SimonHeybrock mentioned this pull request Apr 26, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OwenArnold OwenArnold OwenArnold requested changes

@nvaytet nvaytet nvaytet left review comments

@SimonHeybrock SimonHeybrock SimonHeybrock approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@arm61 @OwenArnold @SimonHeybrock @nvaytet