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

Move flex_table to dxtbx #2113

Merged
merged 27 commits into from
May 18, 2023
Merged

Conversation

toastisme
Copy link
Contributor

This PR along with #521 moves flex_table to dxtbx, with the few reflection_table specific functions (i.e those involving experiment_identifiers) moved to reflection_table_suite.h. The motivation for this is to use flex_table for other purposes in dxtbx (specifically a properties table in Scan), as it's essentially a generic dictionary with some nice functionality that reflection_table extends.

I'm not really happy with the flex_table_suite.h and reflection_table_suite.h setup. At the c++ level you could accidentally use methods from the former and might not notice the lack of experiment identifiers updating. As nearly all flex_table functions are not actually part of the flex_table class, clean inheritance isn't really possible..So I have just copied the convention.

Tests will not pass without #521 being merged first. No additional tests added as all functionality should be the same, and should be covered by tests/algorithms/array_family/test_identifiers_handling.py and tests/algorithms/array_family/test_reflection_table.py

@phyy-nx phyy-nx added the dials_core To review in dials core meeting label Jan 18, 2023
@dagewa
Copy link
Member

dagewa commented Jan 18, 2023

During the DIALS catch up at UCLA we noticed there is some duplicated code in flex_table.h and flex_table_suite.h files in dxtbx and DIALS. Looking back, we realised that this is part of a transfer of the flex table from DIALS to dxtbx, but we have only taken the first step. This PR would complete the process of transfer started by cctbx/dxtbx#521, but it has merge conflicts. Can we get this resolved soon?

@ndevenish
Copy link
Member

Discussed in DIALS core. Probably when this merge is fixed, we'd want to have the header files remain, but just import the dxtbx versions and print a deprecation warning when the old headers are included. This way, downstream dependencies will warning but not break.

@toastisme
Copy link
Contributor Author

Deprecation warnings have been added. I've kept flex_reflection_table.cc pointing at dials/array_family/boost_python/flex_table_suite.h for now just to confirm I've not missed redirecting any functions that might get picked up with tests outside of the CI.

There's currently a build failure for Windows, but this looks to be due to the Windows Azure build using an older version of dxtbx.

ndevenish and others added 10 commits April 6, 2023 15:03
…de of flex_table_suite to integration_integrator_ext.cc.
…de of flex_table_suite to integration_integrator_ext.cc.
…#1903)

Adds damage_series.dose_group_size= option to dials.damage_analysis, to generate some
merging statistics plots for subsets of the data. The corresponding datafiles can also
be output by turning on the options output.damage_series= and
output.accumulation_series=.

This generates additional plots after the pychef analysis which are output in the html
(cc_half, r_pim, i_over_sigma and completeness).

Output files are optionally generated for the damage group series:
    damage_series_0_100.{expt,refl} damage_series_100_200.{expt,refl} etc.
and also optionally for the damage accumulation series:
    damage_series_0_100.{expt,refl} damage_series_0_200.{expt,refl} etc.
And hide conda_base/ -based errors for the entire CI build on Azure.
    towncrier --name=DIALS --version='3.14.0'
Python 3.10 remains the default, for now.

Also, update CI to either disable or suppress PYTHONDEVMODE
warnings from this matplotlib bug:
  matplotlib/matplotlib#25244
@ndevenish
Copy link
Member

ndevenish commented May 16, 2023

Ah, the issue on the CMake is because the unity build merges, everything and the scitbx header scitbx/array_family/boost_python/ref_pickle_double_buffered.h doesn't have a header double-inclusion guard.

In DIALS this was solved by adding a copy of the header that did have an inclusion guard, I will add that here also.

Edit: I am getting confused between this PR and the same branch name on dxtbx. But it's this-issue-related.

@ndevenish
Copy link
Member

Okay, yes, the specific issue is that dxtbx now imports the scitbx/.../ref_pickle_double_buffered.h directly, because the header-inclusion-guard version wasn't moved across. Thus, even though dials only uses the inclusion-guard version, everything inside still gets declared twice. This needs a DXTBX change to fix.

@ndevenish
Copy link
Member

cctbx/dxtbx#636 will fix this, but I will also update the DIALS references

@ndevenish ndevenish merged commit c4e7c13 into dials:main May 18, 2023
4 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dials_core To review in dials core meeting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants