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

Add workflow algorithm for POLDI analysis #649

Merged
merged 29 commits into from Apr 27, 2015

Conversation

MichaelWedel
Copy link
Contributor

This fixes #10702.

Please note that I started on the ticket earlier in a different branch, which was abandoned. Only the commits in this pull request are relevant. For the next release I will create a workflow chart and include it in the documentation.

Testing information
Make sure all tests pass, I fixed a few bugs in some of the POLDI algorithms along the way and had to fine-tune PoldiFitPeaks2D to handle multiple phases.

There is a documentation page for PoldiDataAnalysis including a usage example. Make sure it works, you could also look at the plot that is generated when the option is activated and see that the green curve (difference curve) is not too high compared to the red/black curve. If the PawleyFit option is deactivated, the fit should be faster, but the unit cell information is not generated.

Michael Wedel added 26 commits April 23, 2015 08:01
The first approach to writing a workflow algorithm was too focused on one (complex) use case, which made it way to difficult to use the algorithm for more common cases. After further discussion with the scientists, the scope of the algorithm is better defined.
This will make InitialCell and CrystalSystem of PoldiFitPeaks2D obsolete, as PoldiIndexKnownCompounds now stores the cell and point group.
Previous naming scheme would overwrite indexed workspaces when using a compound multiple times.
This way it's easier to convert between strings/enum in different places.
Point group also has a default now, which is better than having a null pointer around.
These need to be present in the logs of the peak tables now, which they are when PoldiIndexKnownCompounds has been used to generate them. The docs contain a hint what to do when a custom indexing method has been used.
In this first step, workspace groups are accepted, but only the first one is used. Everything still works as expected.
Returning the right workspace which has gone through multiple refinements, making peak criteria more strict.
Updated algorithm category as well.
As a result of the bug fixed in PoldiFitPeaks1D, one unindexed peak that is not actually a peak is excluded, so the numbers need to be adjusted by one.
Fixing space group symbol parsing (did not allow /) and allow rational numbers.
@MichaelWedel MichaelWedel added the Diffraction Issues and pull requests related to diffraction label Apr 24, 2015
@MichaelWedel MichaelWedel added this to the Release 3.4 milestone Apr 24, 2015
@AndreiSavici
Copy link
Member

You should fix your pylint warnings:

  • for unused variables (say a=Load(filename), then unused a) just use dummy_variable_name instead
  • add newlines at the end of the files
  • add a statement to ignore variables defined outside init. Most python algorithms have this

@MichaelWedel
Copy link
Contributor Author

Thanks Andrei, I missed the pylint warnings. But why was the indicator green?

@AndreiSavici
Copy link
Member

For now the threshold is set to 4000 warnings. We need to fix a few more, to slowly get it down

@FedeMPouzols FedeMPouzols self-assigned this Apr 27, 2015
@FedeMPouzols
Copy link
Contributor

All looks good, and extensively documented and tested. I run the system and unit tests locally just in case, without surprises.

Maybe one small thing that could be added to the tests later on is some test cases with wrong properties (users mistype or forget a required property, etc.). And I am not sure if it is possible to have wrong combinations of properties (and/or property values), that could also be added in the unit tests.

I also have a general comment about unit tests, actually forwarding a comment from Owen some days ago on one of my PRs. The idea is to prefer the TSM_... versions of the CxxTest asserts rather than the TS_... adding informative messages wherever it makes sense, so that we can quickly get an idea of what's the issue when a unit test fails. Hum, but I see now that TSM is already used in many places already in these tests ;)

FedeMPouzols added a commit that referenced this pull request Apr 27, 2015
…ithm

Add workflow algorithm for POLDI analysis
@FedeMPouzols FedeMPouzols merged commit 435d4da into master Apr 27, 2015
@FedeMPouzols FedeMPouzols deleted the 10702_poldi_workflow_algorithm branch April 27, 2015 14:14
@MichaelWedel
Copy link
Contributor Author

Thanks for testing thoroughly! You're right, I do not use TSM_ too often, I will pay more attention to this in the future. So far I tried to use them in "bulk" tests where the same check is run for similar data in the same test. Another thing I'm trying to work on is to make smaller, more specific test-cases, which makes it also easier to spot where an error originated, just from the name of the test-case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Diffraction Issues and pull requests related to diffraction
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants