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

T172 tissue and molecular library inconsistency #398

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

LeonieBoland
Copy link
Collaborator

Please check the following before creating the pull request (PR):

  • Did you run automatic tests?
  • Did you run manual tests?
  • Is the code provided in the PR still backwards compatible to previous SIMPA versions?

List any specific code review questions
The classes TissueLibrary and Molecule Library had capitalised global instances before, namely TISSUE_LIBRARY and MOLECULE_LIBRARY. In some examples shown directly at class descriptions, e.g. CircularTubularStructure in structure_library, there is still the old version with the global instance. This has to be changed but the question is if there should be an import in the comment. Other classes used there also don't have an explicit import but the class is imported in the file, e.g. Tags in CircularTubularStructure.
Options are:

  • in the comment from simpa.utils.libraries.tissue_library import TissueLibrary and change TISSUE_LIBRARY.blood() to TissueLibrary.blood()
  • in the comment import simpa as sp and change TISSUE_LIBRARY.blood() to sp.TissueLibrary.blood()
  • in the comment import simpa as sp and change TISSUE_LIBRARY.blood() to sp.TissueLibrary.blood() plus add the sp. prefix in front of all classes even though the import is done in the file
  • just change TISSUE_LIBRARY.blood() to TissueLibrary.blood() and assume that the user knows that the import would still need to be done

List any special testing requirements

Additional context (e.g. papers, documentation, blog posts, ...)

Provide issue / feature request fixed by this PR

Fixes #<172>

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.

1 participant