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

Address TODO comments in code #96

Closed
6 tasks done
bwohlberg opened this issue Nov 16, 2021 · 7 comments · Fixed by #150
Closed
6 tasks done

Address TODO comments in code #96

bwohlberg opened this issue Nov 16, 2021 · 7 comments · Fixed by #150
Assignees
Labels
developer Developer environment: issues related to CI, git, etc.
Milestone

Comments

@bwohlberg
Copy link
Collaborator

bwohlberg commented Nov 16, 2021

There are a number of TODO comments still to be addressed in the code:

  • scico/blockarray.py:1322
  • scico/__init__.py:8
  • scico/numpy/__init__.py:81
  • scico/loss.py:73
  • scico/typing.py:29
  • scico/scipy/special.py:50
@bwohlberg bwohlberg added the developer Developer environment: issues related to CI, git, etc. label Dec 17, 2021
@bwohlberg
Copy link
Collaborator Author

Marking scico/__init__.py as done since that instance is now addressed in #138.

@bwohlberg bwohlberg added this to the Release 0.0.2 milestone Dec 21, 2021
@bwohlberg
Copy link
Collaborator Author

Added this issue to the 0.0.2 release milestone. If it's not possible to completely resolve all of these comments in time for this milestone, at the very least the comments should be moved from the code into individual issues describing the required changes.

@bwohlberg
Copy link
Collaborator Author

Re-opening this issue due to the discovery of yet another TODO lurking in the code:

# TODO: need tests for multi-gpu machines

Someone should give it a look to determine whether it can simply be deleted, or whether a corresponding issue should also be created as a reminder to fulfill the intent of the TODO note.

@bwohlberg bwohlberg reopened this Feb 1, 2022
@lukepfister
Copy link
Contributor

here's a better TODO for that part: consider swapping out the calling mechanism with the host callback we use elsewhere in code, then the multi-gpu stuff comes for free

@Michael-T-McCann
Copy link
Contributor

I spun Luke's suggestion out into #204. We could also add an issue about making tests for multi-GPU systems (generically) but I think that's premature.

@lukepfister
Copy link
Contributor

Also, to give some more context-- that TODO is concerning device placement. So if you have some array x on device 0, and you run the solver, does the result go back to device 0? Per this line

if dev:
it should, but there are no actual tests to check that

I'd also be concerned that this code might not work as expected if the array is sharded across multiple devices, but we don't really deal with that in the library

@bwohlberg
Copy link
Collaborator Author

Closing again because subject of TODO note is now discussed in #204, and the line itself will be removed as part of #202.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer Developer environment: issues related to CI, git, etc.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants