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

test decorator for cpu and cuda #75

Merged
merged 22 commits into from
Jun 17, 2024

Conversation

McHaillet
Copy link
Member

@McHaillet McHaillet commented Jun 17, 2024

Preface: maybe a bit void seeing your other message now, but was fun to mess around with still

Overview of changes:

  • Added a test decorator to run tests on both devices 'cpu' and 'cuda'. It tried with the 'meta' device, but it was too much of a hassle. I don't think torch has a good option yet to do this. Some ideas: we could make a switch to run tests only on CPU for people without a GPU. Or for M1 chips specifically could use this device: https://pytorch.org/docs/master/notes/mps.html. Other thought: Maybe the decorator just runs on all available devices?
  • I add the decorator to all tests and ran them, for some things I made some fixes to run on both devices.
  • Found out midway I did not set up pre-commit properly. I then had to update the pre-commit config and fix some rules for new versions of packages. Sorry about that.

BTW also noticed that 'projection' and 'rescaling' are lacking unittests.

Closes #72

@McHaillet McHaillet requested a review from alisterburt June 17, 2024 16:48
@McHaillet
Copy link
Member Author

I'll respond to your message here :)

Hey @McHaillet ! Amazing - will merge this but just to let you know

I've been factoring out the different functionalities in libtilt into separate packages

torch-image-lerp
torch-grid-utils
torch-fourier-shift
torch-fourier-rescale
torch-subpixel-crop
torch-fourier-slice
torch-fourier-shell-correlation
They're all on PyPI I just haven't moved them into the teamtomo org yet, wanted to add docs first

Didn't realise this!

I'm doing this because I noticed some reticence to depend on/use things in libtilt and a general "unsure what it is/what it does" vibe - so this was a move to lower the bar to using/depending on these functionalities

What would prevent us from releasing a version 0.1 of a libtilt API and putting it on pypi? Maybe add a short text on what the project intention is and a docs page with api. But this is a bit clashing with the individual packages.

Cool that you've popped up, did you want to work on something specific? Would be fun to hack on something if you've got time :-)

I wanted to do some work on this mostly for fun and learn something new, and have a short distraction from writing. I thought updating the tests and making it run on GPU might get this ready for a first version.

Would be a lot of fun to hack something together, but still little time for me atm. But perhaps we could have a quick call or smh to share some ideas?

@McHaillet
Copy link
Member Author

Other thought: Maybe the decorator just runs on all available devices?

Decided to add this in.

@McHaillet
Copy link
Member Author

Would it be possible to make the tests check for differentiability for deep-learning?

@alisterburt
Copy link
Member

this is really wondeful @McHaillet - great work 🙂

What would prevent us from releasing a version 0.1 of a libtilt API and putting it on pypi? Maybe add a short text on what the project intention is and a docs page with api. But this is a bit clashing with the individual packages.

I agree this is one path forwards and did consider it myself, the questions I was asking myself when thinking about it were:

  1. is this the best path for getting the functionality out to people
  2. am I happy with the API

I ended up deciding that small standalone packages would be better on both fronts - the thing that will bring them together and make it feel coherent/cohesive is a nice teamtomo.org which points at their associated docs (which don't exist yet 🤣 )

I'll merge this anyway and will absolutely be adding this to all of those packages. I would love the differentiability tests too but don't know how I would do that immediately, do you have ideas?

Does this make sense to you? Would you like to work together on getting the libtilt functionality into teamtomo as isolated packages? This feels like the equivalent of the v0.1 API you were imagining and it would be great to have someone else excited about the effort 🙂

@alisterburt alisterburt merged commit 9bde07a into teamtomo:main Jun 17, 2024
@McHaillet
Copy link
Member Author

I am not immediately convinced (but I trust you have good ideas on the topic) maybe you could help me see your points?

My main two concerns:

1
Although I fully agree with the teamtomo philo about modular, reusable components, I would say having all these image proc. functions in one place does fulfill this. It's all build on the same torch framework (and some numpy/scipy) which for me makes sense to have in one place.

For a developer that wants to depend on this library, it is also not a huge codebase, i.e. the dependency is fully overshadowed by pytorch. Rather having to add all these individual dependencies seems more tedious.

2
From a POV of developing the library itself, it seems easier to have these things in one place, or at least some parts. As an example:

  • frequency grids seem to belong together with ctf/dose-filters/bandpass.
  • coordinate grids, interpolation, transformations all seem logical to have together.

It would be difficult to first have to go to another package to fix something for CTF's that is due to an issue in frequency grids.

I can see it imposes a hurdle to develop a big library, but it also is a big hurdle if you have to move through different packages and learn how these components tie in together.

I can see the benefit of some splitting but would still be more inclined to keep it one place. If you think splitting is really needed, perhaps some components can still be kept together.

@McHaillet
Copy link
Member Author

I could see though that if we can for example ensure that the grid-utils API (https://github.com/alisterburt/torch-grid-utils/tree/main) remains consistent, it makes it easier to have it as a dependency.

@McHaillet
Copy link
Member Author

McHaillet commented Jun 18, 2024

I'll merge this anyway and will absolutely be adding this to all of those packages. I would love the differentiability tests too but don't know how I would do that immediately, do you have ideas?

Nope! but it should be possible right? You just need to check that the gradients can be calculated from input to output. I don't think its possible as a decorator though.

@McHaillet
Copy link
Member Author

Does this make sense to you? Would you like to work together on getting the libtilt functionality into teamtomo as isolated packages? This feels like the equivalent of the v0.1 API you were imagining and it would be great to have someone else excited about the effort 🙂

For sure down to work together on it! However, my contract is almost finished and I don't know yet where I will be after. For now definitely happy to think along :)

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.

unittesting device agnostic code
2 participants