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

BUG: restore(clear=False) reflections added to other samples #289

Closed
prjemian opened this issue Nov 8, 2023 · 5 comments · Fixed by #292
Closed

BUG: restore(clear=False) reflections added to other samples #289

prjemian opened this issue Nov 8, 2023 · 5 comments · Fixed by #292
Assignees
Labels
Milestone

Comments

@prjemian
Copy link
Contributor

prjemian commented Nov 8, 2023

In [61]: config.restore(pathlib.Path("fourc-config.json"), clear=True)

In [62]: list_reflections(True)
Sample: main

 #   H  K  L   Two Theta    Theta      Chi      Phi   orienting   
 0   2  0  0      37.000    0.000   90.000   71.500   first        
 1   0  4  0      60.000   30.000   90.000    0.000   second       
============================================================================
Sample: EuAl4

 #   H  K  L   Two Theta    Theta      Chi      Phi   orienting   
 0   0  4  0      60.000   30.000   90.000    0.000   first        
 1   2  0  0      37.000    0.000   90.000   71.500   second       
============================================================================

In [63]: config.restore(pathlib.Path("fourc-config.json"), clear=False)

In [64]: list_reflections(True)
Sample: main

 #   H  K  L   Two Theta    Theta      Chi      Phi   orienting   
 0   2  0  0      37.000    0.000   90.000   71.500   first        
 1   0  4  0      60.000   30.000   90.000    0.000   second       
============================================================================
Sample: EuAl4

 #   H  K  L   Two Theta    Theta      Chi      Phi   orienting   
 0   0  4  0      60.000   30.000   90.000    0.000 
 1   2  0  0      37.000    0.000   90.000   71.500 
 2   2  0  0      37.000    0.000   90.000   71.500 
 3   0  4  0      60.000   30.000   90.000    0.000 
 4   0  4  0      60.000   30.000   90.000    0.000   first        
 5   2  0  0      37.000    0.000   90.000   71.500   second       
============================================================================

It looks like when clear=False, it reads reflections from 'main' also into the following sample.

Originally posted by @strempfer in #279 (comment)

@prjemian prjemian changed the title BUG: restore(clear=True) reads reflections into other samples BUG: restore(clear=False) reads reflections into other samples Nov 8, 2023
@prjemian prjemian added the bug label Nov 8, 2023
@prjemian prjemian added this to the v1.1 milestone Nov 8, 2023
@prjemian prjemian self-assigned this Nov 8, 2023
@prjemian prjemian changed the title BUG: restore(clear=False) reads reflections into other samples BUG: restore(clear=False) reflections added to other samples Nov 8, 2023
@prjemian
Copy link
Contributor Author

prjemian commented Nov 8, 2023

@strempfer: I have reproduced this situation in test code with the current version and am working on repairs now. In short, the restored samples should each have two reflections in this test. That fails:

        for sample in e4cv.calc._samples.values():
            assert len(sample.reflections) == 2, f"{sample.name=}"
        assert len(e4cv.calc._samples) == 4

        agent = DiffractometerConfiguration(e4cv)
        config = agent.export()

        agent.restore(config, clear=False)
    
        assert len(e4cv.calc._samples) == 4
        for sample in e4cv.calc._samples.values():
>           assert len(sample.reflections) == 2
E           AssertionError: assert 10 == 2

prjemian added a commit that referenced this issue Nov 8, 2023
prjemian added a commit that referenced this issue Nov 8, 2023
prjemian added a commit that referenced this issue Nov 8, 2023
prjemian added a commit that referenced this issue Nov 8, 2023
@prjemian
Copy link
Contributor Author

prjemian commented Nov 8, 2023

Those commits should have been on a new branch. Since I've pushed them, will remove the changes with git revert.

prjemian added a commit that referenced this issue Nov 8, 2023
This reverts commit aa9b93b.
prjemian added a commit that referenced this issue Nov 8, 2023
prjemian added a commit that referenced this issue Nov 8, 2023
This reverts commit 6c56d1a.
prjemian added a commit that referenced this issue Nov 8, 2023
prjemian added a commit that referenced this issue Nov 8, 2023
prjemian added a commit that referenced this issue Nov 8, 2023
prjemian added a commit that referenced this issue Nov 8, 2023
prjemian added a commit that referenced this issue Nov 8, 2023
prjemian added a commit that referenced this issue Nov 8, 2023
@prjemian
Copy link
Contributor Author

prjemian commented Nov 8, 2023

Lots of new errors now in test_configuration.py module.

@prjemian
Copy link
Contributor Author

prjemian commented Nov 8, 2023

All of the form: It is not possible to compute the UB matrix when the given reflections are colinear. When this line is removed, no such error, but reflections are duplicated:

s.remove_reflection(refl)

@prjemian
Copy link
Contributor Author

prjemian commented Nov 8, 2023

... Starving for a sample.update_reflection() method. Alas, v2 item.

prjemian added a commit that referenced this issue Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant