Skip to content

Conversation

@pc494
Copy link
Member

@pc494 pc494 commented Jan 29, 2020


name: Tidy up streographic code + StructureLibrary changes
about: This deletes some old code and streamlines the StructureLibrary class


Release Notes

major (and breaking)
improvement
Summary: 1 line per end-user relevant change

What does this PR do? Please describe and/or link to an open issue.

  1. Deletes the old streographic library function (superceded by Rotation lists from streographic gridding #55)
  2. Removes the StructureLibraryGenerator and replaces with classmethods.

Describe alternatives you've considered
Discussion on (2)
Previously our syntax involved using a generator that didn't do any "science" to create a library. I feel (@dnjohnstone please weigh in) that

from diffisms.libraries.structure_library import StructureLibrary
StructureLibrary.from_orientation_lists() 
StructureLibrary.from_crystal_systems()

Is a clear syntax, both for end user and developers. To claim what these methods do is "generation" seems a bit rich as it's just combining things with loops. c.f with DiffractionGenerator

Work in progress?
If you know there is more to do, make a checklist here:

  • Catch missing coverage
  • Docstrings
  • Get "philosophical approval"

@pc494 pc494 added the enhancement New feature, request, or improvement label Jan 29, 2020
@pc494
Copy link
Member Author

pc494 commented Jan 29, 2020

@dnjohnstone can I get conceptual approval on this so that I can go tidy up docstrings etc to get it ready to merge?

@dnjohnstone
Copy link
Member

@dnjohnstone can I get conceptual approval on this so that I can go tidy up docstrings etc to get it ready to merge?

Thanks @pc494 - yeah I agree that this is an improvement. I think we got a bit Generator happy at that point.

As an aside, also noticed that we should be documenting Generator classes differently https://docs.scipy.org/doc/numpy-1.15.0/docs/howto_document.html#documenting-generators i.e. have "Yields" rather than "Returns"

@pc494
Copy link
Member Author

pc494 commented Jan 30, 2020

Cool, I'll tidy that up today then.

I'm not so sure about the second part though, I think they mean generators in the formal sense (with yield statements and all) whereas our "Generators" are really just normal classes.

@dnjohnstone
Copy link
Member

Cool, I'll tidy that up today then.

I'm not so sure about the second part though, I think they mean generators in the formal sense (with yield statements and all) whereas our "Generators" are really just normal classes.

Good point, thanks - fuzzy brain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature, request, or improvement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants