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

additions from apstools #87

Closed
wants to merge 18 commits into from
Closed

additions from apstools #87

wants to merge 18 commits into from

Conversation

prjemian
Copy link
Contributor

@prjemian prjemian commented Dec 27, 2020

Add reports from the apstools DiffractometerMixin directly to the Diffractometer class.

Fix #78

Also, fix #86 where Diffractometer.energy did not respond to changes to energy units and/or offset.

Also add Components for geometry_name and class_name (fix #88) so they are recorded with data to enable recovery of UB (#50) from previous runs and resume scanning.

@prjemian prjemian added this to the v0.3.16 milestone Dec 27, 2020
@prjemian prjemian self-assigned this Dec 27, 2020
@prjemian prjemian marked this pull request as ready for review December 27, 2020 22:51
@prjemian
Copy link
Contributor Author

@mrakitin Ready for review now.

@prjemian
Copy link
Contributor Author

The new code here will support #63. Simpler examples can be the result.

@mrakitin
Copy link
Member

mrakitin commented Jan 4, 2021

I am seeing additions from other, earlier PRs. Probably it would make sense to merge them first, then rebase this PR on main to eliminate the overlapping commits.

@prjemian
Copy link
Contributor Author

prjemian commented Jan 4, 2021

The goal here was to bring in the user interface first worked out in apstools (#78). While doing that, two other issues (#86 and #88) were exposed. All these relate back to #50. This is a set of issues that work together. This PR lays foundation for the final work on #50 (to be completed after this PR is merged).

@mrakitin
Copy link
Member

mrakitin commented Jan 5, 2021

Ready to review it once the conflicts are resolved (by git rebase main).

The following merge commit may not be the best solution to eliminate the overlapping commits:

image

I normally rebase.

@prjemian
Copy link
Contributor Author

prjemian commented Jan 5, 2021

@mrakitin Thanks for the suggestions. Evidence is building for a restart on this effort, taking it in pieces (multiple issues, merge conflicts, other PRs that could affect). I'll look at new branches+PRs for the separate issues.

Not closing here. Not yet.

Copy link
Member

@mrakitin mrakitin left a comment

Choose a reason for hiding this comment

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

I have no objections to merge it.

@mrakitin
Copy link
Member

Oh, please disregard my approval, as #116 replaces this PR. Can we close this PR without merging then?

@prjemian
Copy link
Contributor Author

Right. This PR (and another one from January, #89) has been labeled wontfix and the PR has been placed in draft state. Once the replacement PR has been merged, this PR will be checked that no proposed additions were discarded without consideration.

@prjemian
Copy link
Contributor Author

And, PR #116 was just merged.

@prjemian prjemian closed this Apr 19, 2021
@prjemian prjemian deleted the 78-from_apstools branch April 19, 2021 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants