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

Stage 1 remove datablock #504

Merged
merged 9 commits into from Nov 2, 2022
Merged

Stage 1 remove datablock #504

merged 9 commits into from Nov 2, 2022

Conversation

graeme-winter
Copy link
Collaborator

Obviously results in some DIALS test failures, but very few:

Results (1352.51s):
    1517 passed
       3 failed
         - tests/model/test_experiment_list.py:534 test_experimentlist_factory_from_datablock
         - tests/model/test_experiment_list.py:562 test_experimentlist_to_datablock_imageset
         - tests/model/test_experiment_list.py:577 test_experimentlist_to_datablock_centroid_test_data

As promised, making branch to remove DataBlock

Obviously results in some DIALS test failures, but very few:

Results (1352.51s):
    1517 passed
       3 failed
         - tests/model/test_experiment_list.py:534 test_experimentlist_factory_from_datablock
         - tests/model/test_experiment_list.py:562 test_experimentlist_to_datablock_imageset
         - tests/model/test_experiment_list.py:577 test_experimentlist_to_datablock_centroid_test_data
@codecov
Copy link

codecov bot commented Mar 21, 2022

Codecov Report

Merging #504 (f168150) into main (e3c7acd) will decrease coverage by 0.64%.
The diff coverage is 4.16%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #504      +/-   ##
==========================================
- Coverage   40.75%   40.10%   -0.65%     
==========================================
  Files         177      177              
  Lines       15725    15726       +1     
  Branches     2631     2624       -7     
==========================================
- Hits         6408     6307     -101     
- Misses       8745     8861     +116     
+ Partials      572      558      -14     

@phyy-nx phyy-nx mentioned this pull request Mar 25, 2022
7 tasks
@phyy-nx
Copy link

phyy-nx commented Mar 25, 2022

Question: is the dials/util/OptionParser option read_datablocks removed?

@phyy-nx
Copy link

phyy-nx commented Mar 25, 2022

Tasks for me to test these changes:

  • Make a branch in cctbx_project for XFEL CI that pulls the remove-datablock branch from dxtbx and runs the XFEL CI tests
  • Run labelit/labelit_regression tests
  • Code in mewall/lunus (see Deprecate datablocks mewall/lunus#10), LS49, cxid9114, xfel, and xfel_regression all refer to datablocks and need to be updated

@graeme-winter graeme-winter marked this pull request as ready for review March 31, 2022 15:09
@ndevenish ndevenish requested a review from phyy-nx April 12, 2022 08:28
@dwpaley
Copy link

dwpaley commented Oct 21, 2022

I'm working through the repos mentioned above that still use datablocks in some way.

https://github.com/dermen/cxid9114

There is a function datablock_from_numpyarrays that still creates a datablock. It was created in 2019 and it's not used anywhere else in the repo. It is mentioned in a cctbx_project simtbx/diffBragg docstring but not actually used. @dermen I think this PR will not break anything, but datablock_from_numpyarrays should be removed and a few stale docstrings that mention datablocks should be updated.

https://github.com/nksauter/LS49/

There are two types of datablocks in the repo.

In these files, datablocks are mentioned in a docstring but not used in the code:

  • work_pre_experiment/step5_pad.py
  • sim/step6_pad.py
  • sim/step5_pad.py
  • sim/step5_laue.py
  • sim/step4batch_pad.py
  • sim/step4_pad.py
  • sim/step4K_pad.py
  • biocars/laue.py
  • adse13_196/step5_pad.py
  • adse13_196/revapi/step5_pad.py
  • adse13_161/step5_pad.py

In these files:

  • paper1/estimate_gain.py
  • work_pre_experiment/test.py

datablocks are actually used. However both files use a dials ArgumentParser with kwarg read_datablocks, which was removed in dials/dials@747e241 in early 2019. Thus the current PR is not going to break anything that wasn't already broken for several years. @nksauter Do you agree with this conclusion?

Now proceeding to xfel and xfel_regression which could be a bit more extensive.

@dwpaley
Copy link

dwpaley commented Oct 21, 2022

Actually there are no serious problems in cctbx_project or xfel_regression.

This utility script: https://github.com/cctbx/cctbx_project/blob/master/xfel/euxfel/write_new_geom.py [edit: removed after discussion with @asmit3] will become unusable but I don't believe it has been used or tested in several years.

This XFEL GUI log scraper: https://github.com/cctbx/cctbx_project/blob/master/xfel/ui/components/spotfinder_scraper.py looks for files called *datablock.json, but it doesn't actually use datablock code. The associated gui elements were disabled in https://github.com/cctbx/cctbx_project/tree/f5caf41bd03.

Now making the cctbx_project branch mentioned above to run XFEL CI on this dxtbx branch.

@graeme-winter
Copy link
Collaborator Author

@ndevenish propose that we merge this one immediately after you branch for release of 3.x

@nksauter
Copy link

@dwpaley we are OK with respect to LS49. The unit tests run correctly under this PR, and the NESAP benchmark produces correct images (under script https://github.com/nksauter/LS49/blob/bfab44205cbce07713efe53627eccdb55e765785/adse13_196/revapi/saul/827781.sh).

@dwpaley
Copy link

dwpaley commented Oct 24, 2022

This pipeline: https://github.com/cctbx/cctbx_project/runs/9081828838 is running xfel_regression on this branch as @phyy-nx suggested above. If "XFEL CI branch" passes then we'll be all set on our end. Not really expecting any problems directly related to this branch, but there have been a couple recent unrelated xfel_regression changes so I'll keep an eye on this.

@dwpaley
Copy link

dwpaley commented Oct 25, 2022

Got a couple issues but nothing related to datablocks. This is all good on the California end.

@phyy-nx
Copy link

phyy-nx commented Oct 25, 2022

Closes #336

@graeme-winter graeme-winter merged commit 3dba930 into main Nov 2, 2022
@graeme-winter graeme-winter deleted the remove-datablock branch November 2, 2022 09:39
@graeme-winter graeme-winter restored the remove-datablock branch November 2, 2022 10:09
graeme-winter added a commit that referenced this pull request Nov 2, 2022
This reverts commit 3dba930.

Reverting while we revisit the dials regression test failures
@graeme-winter graeme-winter mentioned this pull request Nov 2, 2022
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