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 possibility to set a --skip-modules option without setting a --num-modules #12

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

Conversation

yassirnajmaoui
Copy link
Collaborator

@yassirnajmaoui yassirnajmaoui commented Nov 8, 2024

Changes in this pull request

This fixes a problem that occurs when the user specifies a --skip-module option, but no --num-modules option.
This has been broken since we started using module_indices= in commit c0c9518.
In order to fix this, I had to create a custom class that defines a range that doesn't have a "stop" number, but that can still check if an element is in the range.

This is useful if a user wants to display all the scanner, but also wants to skip a set amount of modules.

Checklist before requesting a review

  • I have performed a self-review of my code
  • I have added docstrings/doxygen in line with the guidance in the developer guide
  • The code builds and runs on my machine

Contribution Notes

Please read and adhere to the contribution guidelines.

Please tick the following:

  • The content of this Pull Request (the Contribution) is intentionally submitted for inclusion in the ETSI software (the Work) under the terms and conditions of the Apache-2.0 License.

@yassirnajmaoui yassirnajmaoui changed the title Add custom range class Add possibility to set a --skip-modules option without setting a --num-modules Nov 8, 2024
@KrisThielemans
Copy link

This seems ok but quite complicated. The following seems to work (but would need a bit more testing):

def openrange(start, step, stop):
     c=start
     while stop == 0 or c < stop:
          yield c
          c += step

module_indices = openrange(0, skip_modules+1, num_modules)

I see though that itertools.islice uses stop=None for "no end condition", so maybe we should do that as well (i.e. while stop is None or ... and default num_modules to None (which is probably the default for argparse already anyway).

@yassirnajmaoui
Copy link
Collaborator Author

yassirnajmaoui commented Nov 8, 2024

This seems ok but quite complicated. The following seems to work (but would need a bit more testing):

def openrange(start, step, stop):
     c=start
     while stop == 0 or c < stop:
          yield c
          c += step

module_indices = openrange(0, skip_modules+1, num_modules)

I see though that itertools.islice uses stop=None for "no end condition", so maybe we should do that as well (i.e. while stop is None or ... and default num_modules to None (which is probably the default for argparse already anyway).

I know it seems complicated for what it's supposed to do, but I haven't found a way to do it with built-in functions that won't do an infinite loop when calling the in operator. I tried itertools.count and it does an infinite loop aswell.

Regarding your code, it doesn't work either, try this:

def openrange(start, step, stop):
     c=start
     while stop == 0 or c < stop:
          yield c
          c += step

r = openrange(0,5,0)

print(0 in r) # Returns True (as it should)
print(5 in r) # Returns True (as it should)
print(11 in r) # Goes through an infinite loop (should be returning False)

@KrisThielemans
Copy link

print(11 in r) # Goes through an infinite loop (should be returning False)

ah, yes, in doesn't work with a generator. Sorry!

I guess they should still be possible by reordering the for loops a bit (a pity we have the rep_module loop!), or other horrible work-arounds. However, this might all be over-engineering. Why not simple add

def get_num_modules(scanner_geometry: petsird.ScannerGeometry) -> int:
    """Compute total number of modules in the scanner"""
    num_modules = 0
    for rep_module in scanner_geometry.replicated_modules:
        num_modules += len(rep_volume.transforms)
    return num_modules

such that you know the upper bound. That function should then be added to petsird_helpers.h later.

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.

3 participants