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

Remove cctbx.xfel dependency from dials.index #2390

Merged
merged 4 commits into from
Aug 11, 2023

Conversation

jbeilstenedmands
Copy link
Contributor

A port of two functions from cctbx.xfel to remove the dependency on cctbx.xfel for stills indexing.

For #2389, #2076

@jbeilstenedmands jbeilstenedmands changed the title Dials indexing cctbx.xfel dependency Remove cctbx.xfel dependency from dials.index Apr 18, 2023
@codecov
Copy link

codecov bot commented Apr 18, 2023

Codecov Report

Merging #2390 (3ee0da6) into main (18146d1) will decrease coverage by 0.03%.
Report is 55 commits behind head on main.
The diff coverage is 81.16%.

❗ Current head 3ee0da6 differs from pull request most recent head 81ad379. Consider uploading reports for the commit 81ad379 to get more accurate results

@@            Coverage Diff             @@
##             main    #2390      +/-   ##
==========================================
- Coverage   78.62%   78.59%   -0.03%     
==========================================
  Files         603      608       +5     
  Lines       73626    74539     +913     
  Branches    10002    10140     +138     
==========================================
+ Hits        57885    58585     +700     
- Misses      13611    13787     +176     
- Partials     2130     2167      +37     

@phyy-nx
Copy link
Member

phyy-nx commented Apr 20, 2023

Hi @jbeilstenedmands I have an alternate approach I'm working on. I'll show it off in tomorrow's dials core if that's ok. Thanks!

@jbeilstenedmands
Copy link
Contributor Author

Hi @phyy-nx , no problem, my idea was to get this out in the open for discussion at the core meeting, happy to consider alternative approaches.

@graeme-winter
Copy link
Contributor

I will note that the fundamental issue is however circular dependencies. Any effort to make the graphs acyclic will be much appreciated.

@phyy-nx
Copy link
Member

phyy-nx commented Apr 22, 2023

Conversation here: #2076 (comment)

@jbeilstenedmands
Copy link
Contributor Author

Closing, as #2404 will provide the solution to the issue

@ndevenish
Copy link
Member

After internal discussion we decided that we'd like to reconsider merging this, so that we can get e.g. serial tooling working on conda builds - and to revert when the alternate serialtbx stuff is ready to go.

To be discussed in the meeting later this week, I guess.

@ndevenish ndevenish reopened this Aug 8, 2023
@ndevenish
Copy link
Member

After discussion at DIALS-core, we've agreed to merge this for release, but to add the immediate revert to #2404 so that it's undone when serialtbx is ready.

@ndevenish ndevenish merged commit 867d1d6 into main Aug 11, 2023
21 checks passed
@ndevenish ndevenish deleted the ssx_index_xfel_dependency branch August 11, 2023 08:36
ndevenish added a commit that referenced this pull request Aug 11, 2023
toastisme pushed a commit to toastisme/dials that referenced this pull request Aug 21, 2023
* Port green_curve_area function
* Port nave mosaicity minimizer

It's agreed that this will be reverted as part of moving to serialtbx
phyy-nx added a commit that referenced this pull request Sep 26, 2023
DIALS and dxtbx have had dependencies on cctbx_project/xfel, and vice versa. This PR is one of 8 that will break the cyclic dependency.

Notably for DIALS, this specifically moves sublattice_helper from xfel to dials.algorithms.integration and removes:
@pytest.mark.xfel

For more detail, see cctbx/cctbx_project#872 and cctbx/dxtbx#627

Closes #2076 and #2389

Commits:
* max_like moved from xfel to serialtbx
* Time code moved from xfel to serialtbx
* green_curve_area and ewald_proximal_volume moved to serialtbx
* sublattice_helper moved in from xfel. Within dials.stills_process, create negative control for background modeling or validation.
* Change import of sublattice_helper
* ConstructFrame moved to serialtbx
* Remove check for xfel in tests
* Fix import
* Remove pytest xfel mark
* Temporarily checkout dxtbx and cctbx serial_tbx branches
* Revert "Remove cctbx.xfel dependency from dials.index (#2390)"
* Revert "Temporarily checkout dxtbx and cctbx serial_tbx branches"
* Use latest CCTBX nightly for prebuilt

---------

Co-authored-by: Nicholas Sauter <nksauter@lbl.gov>
Co-authored-by: Nicholas Devenish <ndevenish@gmail.com>
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.

None yet

5 participants